Yes, it should go into 1.18. I think that a new JIRA case needs to be logged for this change, so that it is clearly Avatica-specific. This change relates to order of fields in Meta with respect to Jackson, but it has nothing to do with JavaTypeFactory (which is a Calcite concept, not an Avatica concept).
On Wed, Feb 17, 2021 at 3:08 PM Francis Chuang <[email protected]> wrote: > > Hey Alessandro + all, > > Do you think it would be possible to get this into the next Avatica > release (1.18.0)? I note that that PR #138 also has a corresponding PR > for Calcite, which would require this PR to go into Avatica if we want > the corresponding PR for Calcite to go into the next Calcite release. > > Francis > > On 16/02/2021 10:15 am, Alessandro Solimando wrote: > > Hi again, > > sorry I have hit "send" too soon, here is the PR: > > https://github.com/apache/calcite-avatica/pull/138 > > > > Best regards, > > Alessandro > > > > On Mon, 15 Feb 2021 at 23:46, Alessandro Solimando < > > [email protected]> wrote: > > > >> Hi Julian, > >> sorry for the late reply. > >> > >> It's true that with third-party classes annotations are not viable, and > >> that's an important use case. > >> > >> Regarding the test case and the user experience, I like the idea of making > >> the strategy configurable, I'd capture the strategies you have listed via > >> an enum as follows. > >> > >> public enum FIELDS_ORDERING { > >> JVM, > >> ALPHABETICAL, > >> ALPHABETICAL_AND_HIERARCHY, > >> EXPLICIT, > >> EXPLICIT_TOLERANT, > >> ANNOTATIONS > >> } > >> > >> where EXPLICIT and EXPLICIT_TOLERANT expects a second mandatory parameter > >> that is the list of fields. > >> EXPLICIT fails if the list does not fully cover the public fields of the > >> class, EXPLICIT_TOLERANT does not, and use to incomplete list to "project" > >> (i.e., all and only fields appearing in the list will be present in the > >> derived SQL type). > >> > >> An example of the schema.json: > >> schemas: [ > >> { > >> name: "author", > >> type: "custom", > >> factory: > >> "org.apache.calcite.adapter.java.ReflectiveSchema$Factory", > >> operand: { > >> class: "org.apache.calcite.test.Author", > >> orderingStrategy: "EXPLICIT_TOLERANT", > >> fields: [“aid”, “name”, “birthplace”] > >> } > >> } > >> ] > >> > >> Regarding ANNOTATION, I have tried the "-parameters" build option > >> suggested by Vladimir, and it works well. > >> > >> In case the parameter names are not available (i.e., the target class was > >> not compiled with the sought parameter) I suggest we fail the conversion > >> (this is an advanced use-case, the user should know about how the classes > >> are compiled). > >> > >> In case of multiple constructors for the target class, we could either: > >> 1) pick the (valid) constructor having the biggest overlap with the class > >> public fields, or > >> 2) pick the first (valid) constructor > >> > >> With "valid" constructor I mean a constructor where each parameter > >> matching a public field, has a type assignable to that of the homonym > >> field. > >> > >> Of course there are some details and corner cases left to handle, but > >> that's probably for the PR itself. > >> > >> As I said in the first email, some changes are required on the Avatica > >> side to completely remove Class.getFields() and Class.getDeclaredFields(). > >> Such changes are independent from the strategy we will adopt on Calcite, > >> they just allow us to "pass" the field ordering to the JDBC enumerator and > >> other classes which need to be aware of the field order. > >> Despite them being minimal (you can check here > >> <https://github.com/asolimando/calcite-avatica/compare/master...asolimando:master-CALCITE-2489-createStructTypeUnstable#diff-651ecd95723b9508c17cfd021ec3894dfbdc81ea3ce9384a5d57cf209a214a97>) > >> I am afraid it's late for 1.18.0 (just saw Francis' email), I have > >> nonetheless opened a PR, just in case it's doable: > >> > >> Best regards, > >> Alessandro > >> > >> On Tue, 9 Feb 2021 at 23:02, Julian Hyde <[email protected]> wrote: > >> > >>> A very important use case is when you are basing a table on third-party > >>> class and you are not able to add annotations. People using these classes > >>> should be able to tell the reflective adapter (that’s not its real name, > >>> but you know what I mean) how to derive a stable ordering for the fields. > >>> > >>> There are several possible strategies: > >>> > >>> Use the natural order from the JVM. (Yes, let people do dumb things.) > >>> Order fields alphabetically > >>> Order fields alphabetically, putting inherited fields (from base classes) > >>> first > >>> Order using a list of field names, e.g. [“aid”, “name”, “birthplace”, > >>> “books”] and a boolean saying whether to fail if the list is not > >>> exhaustive. > >>> Look for annotations > >>> > >>> These strategies would be in the schema.json file (or however the schema > >>> is configured). In my opinion, that is preferable to adding annotations. > >>> > >>> I’d approach this by designing the user’s experience. That is, write the > >>> test case for the adapter. Once we have agreed the specification, the > >>> implementation is straightforward. > >>> > >>> Julian > >>> > >>> > >>>> On Feb 9, 2021, at 2:35 AM, Vladimir Sitnikov < > >>> [email protected]> wrote: > >>>> > >>>> Alessandro, thanks for pushing this. > >>>> > >>>> Frankly speaking, I don't like @CalciteField(order=42) approach. It > >>> looks > >>>> like annotating the constructor parameters makes more sense, and it is > >>>> more flexible. > >>>> A similar case is covered in Jackson with "parameter names" module: > >>>> > >>> https://github.com/FasterXML/jackson-modules-java8/tree/master/parameter-names > >>>> Just in case, Java8+ can include parameter names to the reflection info > >>> in > >>>> case user supplies `-parameters` option for the compiler. > >>>> > >>>>> NewExpression > >>>> > >>>> It might indeed be a too low-level API for that. > >>>> Could you please highlight which test relies on the order of constructor > >>>> arguments? > >>>> > >>>> By the way, SQL structs rely on the order of the fields rather than > >>> field > >>>> names. > >>>> So it might be ok to rely on the order of the constructor arguments. > >>>> > >>>> Vladimir > >>> > >>> > >
