Fix failing Calcite unit tests with existing work-a-round for calcite property setup

[capistrant]

Description

Issue #4909 seems to have popped up again. At least for me. I applied the fix from #5451 liberally to all new Calcite test classes introduced in #9279 to fix on my local. Maybe @nishantmonu51 can speak to if my solve properly mirrors his initial fix.

On my local I can reproduce each time by building the sql module without these changes and then remediate by applying these changes. As I said above, I liberally slapped the class extension on each new class that I identified in Calcite as I was working on a different effort when this popped up after merging master into my branch. Hoping Nishant can quickly identify if my application was overkill and I can remove in un-needed locations.

---

This PR has:

• been self-reviewed.

---

Key changed/added classes in this PR

N/A... Just some small unit test changes.

[suneet-s]

Strange... I couldn't get these tests to fail locally. I tried running them one by one, but that didn't isolate the problem either. I wrote these tests to use mocks, so it shouldn't really rely on any Calcite system properties being set

This is how I ran my tests mvn -Dtest=NamedDruidSchemaTest test

[capistrant]

@suneet-s what if you just run mvn test on the whole sql module? Your tests aren't actually failing for me but tests later on are. It is like calcite is setting up properties once and if the tests run in a certain order on a certain system, everything gets out of whack. But, that may work just fine on your environment... as in the issue referenced above, Gian seemed to have never seen it while the 2 others involved in the issue communications did see it. So dependent on underlying environment of developer executing tests I guess.

I did some analysis on master over lunch that seemed to have narrowed it down to CalcitePlannerModuleTest:

##PASS

• mvn -Dtest=<test> with each of the new test classes from Guicify druid sql module #9279
• mvn -Dtest=DruidCalciteSchemaModuleTest,CalciteQueryTest test
• mvn -Dtest=NamedDruidSchemaTest,CalciteQueryTest test
• mvn -Dtest=NamedLookupSchemaTest,CalciteQueryTest test
• mvn -Dtest=NamedSystemSchemaTest,CalciteQueryTest test
  ##FAIL
• mvn test
• mvn -Dtest=CalcitePlannerModuleTest,CalciteQueryTest test

[suneet-s]

@capistrant Thanks for the repro steps. When I wrote the tests, mvn test failed locally for a couple classes where it was complaining about something like this

org.apache.calcite.sql.validate.SqlValidatorException: Cannot apply <> to the two different charsets UTF-16LE and ISO-8859-1

but it was in a class I didn't write, and they passed on Travis, so I didn't dig in to why they were failing locally. I wonder if we should be unsetting the system properties that CalciteTestBase is setting in every test. That way, we can trust that each test is running in isolation.

[capistrant]

@suneet-s

I looked into this a little bit and I think I'm okay with Nishants approach to fix this with the CalciteTestBase class. It makes sense why he did it, since once the Calcite classes get loaded up and read the calcite specific System properties for configuration, they are stuck like that for the life of the JVM. This was an easy solve to make sure that the system properties were correct for the life of the JVM. It is not obvious to developers to add this unless they reference other tests that touch Calcite that they should extend his base class, which sucks. I'm not sure there is a way to configure Calcite without using System properties and Calcites.java seems to indicate the same in a from the comments in Calcites.setSystemProperties().

TravisCI passing despite all of this has me scratching my head. Maybe the tests are running in a different order than on local..? Or someone in the past messed with the configuration to get past this?

I personally don't see the current approach as a problem. It seems like more of an inconvenience. However, if you have thoughts on a better solution, I think anyone would welcome it because what we currently have here stinks

[suneet-s]

@capistrant That makes sense. I tried for a little bit to find a cleaner solution, but haven't found a working solution yet.

It looks like the CI failures are flaky. Maybe one of the committers can re-trigger the failed tests.

[capistrant]

I created #9333 in case I or someone else is able to circle back on this and see if there is a better way to tackle the problem than the current solution.

[suneet-s]

Sorry, I thought I approved this a few days ago...

[clintropolis]

This fixes running tests locally for me 👍