[SPARK-15954][SQL][PySpark][TEST] Fix TestHiveContext interaction with PySpark issue

[holdenk]

What changes were proposed in this pull request?

SPARK-15745 made TestHive unreliable from PySpark test cases, to support it we should allow both resource or system property based lookup for loading the hive file.

How was this patch tested?

Existing PySpark tests now pass.

[holdenk]

cc @MLnick I factored out the test fix from the doc change PR.

[SparkQA]

Test build #60709 has finished for PR 13737 at commit 330c499.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[holdenk]

re-ping @MLnick and cc @jkbradley / @rxin / @sameeragarwal - see the parent PR at #12938 for some discussion around the fix.

[sameeragarwal]

Thanks, LGTM. It'd be great to add a comment here about the fallback and its implications on pyspark tests to prevent future regressions. Also, out of curiosity, do we know what the underlying cause of flakiness is?

[rxin]

Why does Python need to load these test resources? I think the proper fix is to get rid of that dependency. Otherwise we are making the test harness more and more complicated and tighter coupling.

[holdenk]

@sameeragarwal I'm not super sure but my guess is that the resources are cleaned up at some point during the testing (and in the local dev build isn't normally packaged).

As for removing the test resources requirement from PySpark: How about I look at doing that in a follow up PR since for now its difficult for PySpark developers to test and this is blocking another outstanding PR?

[holdenk]

@sameeragarwal updated the comment

[SparkQA]

Test build #60894 has finished for PR 13737 at commit 4dd92c0.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[MLnick]

@rxin from what I can see, the "quick" fix of trying to make say testTables and hiveQTestUtilTables a lazy val won't work due to HiveContextSQLTests.test_save_and_load_table calling spark.sql which ultimately uses testTables in TestHiveQueryExecution.analyzed.

So the fix will likely be a little more involved, perhaps as @holdenk says it's best to go ahead with this PR and do the decoupling separately? Just given where we are in the release cycle.

[MLnick]

ping @rxin - I think we should fix this before cutting a new RC

[rxin]

Can you check what files the python tests are using? My understanding is that there should only be a few number of places in Python that use these test files.

[holdenk]

@rxin Temporarily disabling registration it seems the files aren't actually used from the Python tests, rather the hiveQTestUtilTables are eagerly evaluated and registered as pointed out by @MLnick. I could change this to add a flag to skip initialization and registration of these tables of these for the Python tests instead if you would prefer that fix (although I'm not sure if that is a better fix per-se).

[rxin]

I'm suggesting just removing the dependency on TestHiveContext in Python altogether. It shouldn't be that difficult. Don't add hacks just to work around some init, if we can get rid of the problem.

[rxin]

BTW Jenkins seems to be fine with Python? What's the problem?

[holdenk]

@rxin ah sorry, I though you wanted to remove the dependency around the file loading functionality - that makes a bit more sense. I can take a look at that possibility in more detail tomorrow if we don't want the simple fix.

At first glance the scala TestHiveContext is used inside of _createForTesting in PySpark which is used by HiveContextSQLTests). From what I remember of working in Spark Testing Base there is some useful bits done inside of the hive testing context base to allow them to work with a test metastore (but that could be a bit out of date).

I'm not convinced removing the Python dependency on the TestHiveContext is the best way to fix this problem which seems to have been introduced just to allow the Hive tests to be used in more places but I'll investigate that more tomorrow.

The problem seems to be that the files aren't always packaged as resources when you build locally (and in Jenkins it was somehow a race condition some of them time). It seemed better to try and get a small working fix in quickly and do the deep dive later.

[MLnick]

@rxin @holdenk the test is called HiveContextSQLTests - implying to me that a dependency on TestHiveContext is kinda required? Are these tests actually not testing Hive/HiveContext anymore? In which case the suggestion of removing dependency works ... and maybe the test should be renamed?

[MLnick]

@rxin try running ./python/run-tests locally. It fails for me. Even if ./dev/run-tests might pass because of packaging/timing, I think having ./python/run-tests fail in a release is not acceptable.

[holdenk]

@MLnick @rxin yah so after poking at it a bit today I don't see a good way to disentagle this - we presumably want to make sure that PySpark works well with a hive based Spark Session (even if the tests weren't testing hive specific functionality).

If we would rather fix it by disabling the loading of tables from Python - I did make some changes so that we could disable loading the test tables / required files for Python based tests (which might be good sort of regardless since loading the files presumably takes some time and the Scala test tables aren't used in the Python tests).

[rxin]

Ah OK. I just looked into this and submitted a fix: #14005

[holdenk]

Closing this since it seems like @rxin has a fix.