Hello all, Thanks Francis for your message. I have filed CALCITE-4503 <https://issues.apache.org/jira/browse/CALCITE-4503> as suggested by Julian. I have filled "fix version" as 1.8.0 since we target this release for the PR, I hope it's OK.
For the open PR I have pushed a new commit with a commit message associated to the new JIRA case, I haven't amended the original commit to avoid problems with the on-going review, this can be safely done during the final "squashing" I think. Best regards, Alessandro On Thu, 18 Feb 2021 at 01:05, Julian Hyde <[email protected]> wrote: > 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 > > >>> > > >>> > > > >
