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.solima...@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 <jhyde.apa...@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.vladi...@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
>>
>>
