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
>
>
