[CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructTyp…

[asolimando]

…e is unstable

The changes in the PR are meant to allow us to "pass" the field ordering and avoid any call to Class.getFields(), which is problematic as it's JVM dependent.

More details are available in the ML discussion

EDIT: let me provide a more detailed description/overview to ease the reviewing process.

The PR introduces a deprecation annotation for method

public RecordIteratorCursor(Iterator<E> iterator, Class<E> clazz) {
    this(iterator, clazz, Arrays.asList(clazz.getFields()));
}

This is necessary to adopt a Calcite-specific and deterministic/stable fields ordering.
This is possible by passing the fields (in the sought order) via RecordIteratorCursor(Iterator, Class, List), as already done for record with projected columns (in order to know what to keep/drop a List of fields is explicitly passed).

In order to use the same method for generic records (projected and non-projected), Meta.java had to be updated, to make some methods more generic (they were "hard-coding" Style.RECORD_PROJECTION as it was the only expected user of those methods, while now we want a Style parameter to accommodate both Style.RECORD_PROJECTION and Style.RECORD).

The only "glitch" is that MetaImpl internal classes (to expose different metadata, the catalog etc.) have columns not matching the field names (they are in capital letters, etc.).
On Calcite side it suffices to pass null instead of the class name for those classes (it's a single call in Calcite, I have already tried that locally), so the code won't try to validate the column names against the class fields (only done when a class is provided, it's a "sanity check", but the precondition is to have obtained the column names from the class fields, which is not the case for those internal classes).

So, as per the comment in the code, once we merge CALCITE-2489 in Calcite codebase we can drop the ugly if statement.

Changes in MetaImpl.java, are simply taking advantage of the more general signature of some methods in Meta.java, where the code had to differentiate between Style.RECORD_PROJECTION and Style.RECORD, now we can call a single method.

[F21]

Discussion for this change is available on the mailing list here: https://lists.apache.org/thread.html/r7537572f56fe2d734baad2de7fdeda490deb520febc3072c60e79de0%40%3Cdev.calcite.apache.org%3E

[asolimando]

I realized that there were no tests in Avatica specifically covering the lines changed by the PR.
Even if the changed code is validated indirectly by the Calcite tests, I felt it was better to have unit-tests in Avatica too for that, the last commit adds them.

[asolimando]

Rebased on current master

[F21]

ping @julianhyde

[julianhyde]

Sorry, no, I'm not planning to commit this. I comment on a lot of changes; that doesn't automatically make me the committer.

[zabetak]

Thanks for the PR @asolimando ! Left a few small comments and tried to resolve them at the same time in https://github.com/zabetak/calcite-avatica/tree/CALCITE-4503. Can you check the changes and let me know what you think.

[zabetak]

@asolimando Regarding the change that is neccessary to do in Calcite for the tests to pass do we need to wait for CALCITE-2489 or it can be done independently?

[asolimando]

@asolimando Regarding the change that is neccessary to do in Calcite for the tests to pass do we need to wait for CALCITE-2489 or it can be done independently?

The fix does not depend on CALCITE-2489 and can be applied independently, it just boils down to replacing clazz with null at this line in Calcite:
https://github.com/apache/calcite/blob/4bc916619fd286b2c0cc4d5c653c96a68801d74e/core/src/main/java/org/apache/calcite/jdbc/CalciteMetaImpl.java#L200

I can file a JIRA ticket in Calcite for that and prepare a PR so we are ready for Avatica 1.18.

[zabetak]

I can file a JIRA ticket in Calcite for that and prepare a PR so we are ready for Avatica 1.18.

Great let's do that and I will take it from there (rebase and squash everything along with the extra commits and push to master).

[asolimando]

I can file a JIRA ticket in Calcite for that and prepare a PR so we are ready for Avatica 1.18.

Great let's do that and I will take it from there (rebase and squash everything along with the extra commits and push to master).

Thanks a lot @zabetak, much appreciated. I have filed CALCITE-4556 and associated PR which is already passing the tests in Calcite, it does not even need to wait for the fix on Avatica's side.

[zabetak]

@asolimando it seems there are more failures in Calcite: https://github.com/zabetak/calcite-avatica/runs/2216366873?check_suite_focus=true#step:5:468

Can you please have a look?

[asolimando]

@asolimando it seems there are more failures in Calcite: https://github.com/zabetak/calcite-avatica/runs/2216366873?check_suite_focus=true#step:5:468

Can you please have a look?

That's weird, I will have a look asap, it looks like an array-based cursor is used even for non-empty metadata result sets, instead of a record-based one, maybe one of the latest changes is causing that as a side-effect.

Btw, I am having issues lately to use the sources from my local calcite-avatica project in Calcite in IntelliJ.

I usually uncomment in gradle.properties the line localAvatica=../calcite-avatica, Avatica is correctly imported into IntelliJ, and it's the version that is used. Now, the importing bit works, but when it comes to debug the unit-tests, it's using 1.27.0 in the local maven cache. I did not have this problem before, I don't know where this is coming from, am I the only one experiencing that?

[asolimando]

Hi @zabetak, I have fixed the issue and opened a PR against your branch, you can find it here: zabetak#1