Use DatabaseMetadata#getTables API in JdbcExecutor to obtain tables/views

[zabetak]

Currently JdbcExecutor contains two abstract methods to obtain tables and views from the underlying JDBC database and requires adding new subclass for each different database.

I think we could use the standard JDBC API DatabaseMetadata#getTables (https://docs.oracle.com/javase/8/docs/api/java/sql/DatabaseMetaData.html#getTables-java.lang.String-java.lang.String-java.lang.String-java.lang.String:A-) to achieve the same purpose.

That way we may be able to run in a wide variety of JDBC endpoints without requiring adding a separate class every time.

[zabetak]

WDYT @mbudiu-vmw ?

[mihaibudiu]

In fact that's the API used by the HsqldbExecutor.
I will make this the default implementation in the base class.
But even the HsqldbExecutor has to removethe "PUBLIC" table from the list of tables. I wonder whether it could be filtered using some other arguments to DatabaseMetadata#getTables

[mihaibudiu]

I will add this as a commit to #25. I hope that's fine.

[zabetak]

If I understand well we need the list of tables/views in order to ensure that each test will run with a "clean" DB state.
If that's the only use case then we may just need to save the list of tables/views before the first test run (initState) and then just ensure that we do the proper cleanup after each test by dropping anything created afterwards (currentState - initState).

[mihaibudiu]

Yes, that sounds like a good way to do it.
I will implement that.
Any other fixes I should do? It's unclear to me whether @julianhyde's comments must be implemented now or can be left for a future version.

[mihaibudiu]

Actually, there is a problem with this approach: let's say you run some tests and they fail with an exception.
Then the test database will be left with some tables and views.
So when you run again you can't assume that these tables are part of the catalog.

[julianhyde]

My comments are only advisory. Feel free to disregard, or defer them, because you are an equal partner in this project. For example, I think that javadoc for public methods is a practice we should follow, because if we don't do it now, future contributors will follow our example. I added checkstyle for a similar reason.

My only ask is that before 0.2 we have an API that Calcite can use.

[mihaibudiu]

I think I have commented everything public.

[julianhyde]

OK, that was just an example. I'm not going to veto commits. I am going to make suggestions that I think benefit the long-term health of the project. I probably won't have time to review every commit, but I'm happy to make release 0.2 when you're ready.

[mihaibudiu]

I understand the importance of following good coding practices and I will strive to follow them.
As far as I know this should be a good 0.2 release, I hope it will enable us to build a CalciteExecutor in a different package.

[julianhyde]

How about building the CalciteExecutor in Calcite? I think you could start by trimming away most of apache/calcite#3145 but leaving the CalciteExecutor. Over time (i.e. in release 0.3) some of the code in CalciteExecutor could be moved to a base class in sql-logic-test.

[zabetak]

I share Julian's viewpoint; I make comments and suggestions that hopefully will make the project easier to use and adopted but I am not insisting on how things should be done and when.

Also when I open issues, I don't imply that Mihai or Julian should do the work. For instance, I could also take some tasks as this one in order to help out or we could even leave some open for other contributors who may wish to contribute to the project.

Now regarding the problem below, we should be able to address it by using try-catch blocks and do the necessary cleanup in finally as it is usually done in many occasions when we want to guarantee proper release of resources.

Actually, there is a problem with this approach: let's say you run some tests and they fail with an exception.
Then the test database will be left with some tables and views.
So when you run again you can't assume that these tables are part of the catalog.

[mihaibudiu]

I already have written the CalciteExecutor in Calcite, and I have the code ready to submit.
It's only 2 Java files.
But it won't compile without the 0.2 release of SLT.