Re: Discussion around CALCITE-2489 (risks linked to Class.getFields())

[Alessandro Solimando <al...@gmail.com>]

Hello everyone,
I have finally managed to brush up the implementation I did several months
ago and opened a PR: https://github.com/apache/calcite/pull/2488

I have followed the main ideas from this discussion, so I think the overall
structure is sound already, but the devil lies in the details.

Looking forward to hearing the comments from the community!

Best regards,
Alessandro


On Thu, 18 Feb 2021 at 09:11, Alessandro Solimando <
alessandro.solimando@gmail.com> wrote:

> 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 <jh...@apache.org> 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 <fr...@apache.org>
>> 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 <
>> > > alessandro.solimando@gmail.com> 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 <jh...@gmail.com>
>> 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 <
>> > >>> sitnikov.vladimir@gmail.com> 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
>> > >>>
>> > >>>
>> > >
>>
>