vlsi commented on PR #2488:
URL: https://github.com/apache/calcite/pull/2488#issuecomment-6113508853
I suggest closing this PR, and either resolving CALCITE-2489 as "Won't Fix"
or narrowing it to an opt-in ordering. The PR fixes an instability nobody has
reported, and in exchange it changes the column order of existing reflective
tables on upgrade.
**The instability has not been observed.** `Class.getFields()` has no
specified order, but HotSpot 8, 11, 17, 21 and 25, OpenJ9 17 and 21, and
GraalVM 17 all return public instance fields in declaration order, subclass
fields first. JIRA has no report of a reflective table whose columns came out
in an unexpected order: CALCITE-2489, CALCITE-4436, and CALCITE-4503, and the
review comments on CALCITE-2404, all come from reading the code. On
CALCITE-2489 Julian wrote: "I doubt that anyone has ever suffered from this
issue in the real world." The 2021 dev@ thread observed the same: when a plan
rebuilds a row of the table's class, generated code calls `new C(...)` with
arguments in `getFields()` order, so a JVM that reordered fields would already
break those queries, and none has been reported.
**The new default changes column order for existing users.** `CONSTRUCTOR`
falls back to alphabetical order, base-class fields first, whenever the class
is compiled without `-parameters` (javac, Maven, and Gradle leave it off by
default) or has no constructor whose parameters name its fields. Take a typical
row class:
```java
public class Order {
public final int id;
public final String customer;
public final double amount;
public Order(int id, String customer, double amount) { ... }
}
```
Today its columns are `id, customer, amount`. With this PR they become
`amount, customer, id`. After an upgrade, with no change on the user's side:
- `select * from orders` returns the columns in a different order, so any
code that maps a `ResultSet` by position breaks.
- `rs.getInt(1)` returns `(int) amount` instead of `id`. Avatica converts a
`DOUBLE` to `int` silently, so there is no exception, only wrong data.
- `select * from orders union all select * from archive.orders`, where
`archive.orders` is a JDBC table with `id, customer, amount`, still validates:
column 1 is derived as `DOUBLE` and mixes amounts with ids. `insert into t
select * from orders` misaligns the same way.
- `order by 1` in queries and in model views now sorts by `amount`.
- Tests that pin plans see different input references; this PR had to
rewrite the `depts2` plan in `MaterializedViewRelOptRulesTest` (`FLOOR($t3, …)`
became `FLOOR($t7, …)`, and the join key `$4` became `$3`).
- `select * from "metadata".COLUMNS` starts with `bufferLength,
charOctetLength, columnDef, columnName` instead of `tableCat, tableSchem,
tableName`. Only `MetaTable` is special-cased.
- Classes compiled with `-parameters` (Spring Boot's Gradle plugin and its
Maven parent POM turn it on) get constructor order instead: in
`testDateColumn`, `hireDate` of `EmployeeWithHireDate` moves from the first
column to the last. Column order therefore starts to depend on a compiler flag
of the user's build, and turning that flag on or off reorders the columns again.
- A Geode region whose value class has private fields loses those columns,
because `JavaTypeFactoryExtImpl` now keeps public fields only; queries that
name them fail with "Column ... not found".
**The opt-in modes return wrong rows.** Under `EXPLICIT`, generated code
still resolves column i through `Types.nthField`, which uses `CONSTRUCTOR`
order, while the row type follows the `fields` list. `select "name", "empid"
from emps where "empid" > 100` throws `ClassCastException: Float cannot be cast
to Integer`; under `EXPLICIT_TOLERANT`, `select "empid" from emps where "empid"
> 100` returns 0 rows instead of 3. With `"fieldsOrdering": "ALPHABETICAL"` and
no `fields`, the row type stays in constructor order while `scan()` returns
values alphabetically.
**The branch is far behind.** It conflicts with main in 11 files, including
`ReflectiveSchema.Factory` (`@ReflectiveSchema.Input`, CALCITE-7712) and the
record handling in `ConstantExpression` (CALCITE-6244).
If someone needs a stable order, a new PR could keep `getFields()` order as
the default and add the strategies from the 2021 dev@ thread as opt-in model
operands, with every ordinal lookup (`nthField`, `RecordEnumeratorCursor`,
`TableScanNode`) reading the same ordering as the row type. Java records are a
separate and real gap: their fields are private, so a record-backed table in
`ReflectiveSchema` has no columns today, and `Class#getRecordComponents()`
gives them a defined order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
