924060929 commented on PR #67166:
URL: https://github.com/apache/doris/pull/67166#issuecomment-5520315128
## Compatibility Review: Struct Field Name Lookup Risks
Thanks for the detailed work on preserving nested field case. I ran targeted
compatibility tests simulating locale-dependent metadata persistence and
replay. Below are findings from code tracing and Java-level verification.
### Issue 1 — `catalog.StructType.getField()` has no legacy fallback (P1)
The PR adds a `legacyLocaleDependentName` fallback in
`nereids.types.StructType.getField()`, but the **catalog**
`StructType.getField()` (which is the persistent type, deserialized via Gson
with `@SerializedName` on `fieldMap`) does **not** get a similar fallback.
After Gson deserialization, `fieldMap` keys are whatever the old FE
persisted — they are **not** rebuilt by the constructor. On a non-ROOT locale
(e.g. Turkish `tr-TR`):
```
Old FE (Turkish): "I".toLowerCase() = "ı" (U+0131) → fieldMap key = "ı"
New FE (ROOT): getField("I") → fieldMap.get("I".toLowerCase(ROOT)) =
fieldMap.get("i") → MISS
```
I verified this with a standalone Java test — the catalog `getField()`
returns `null` for fields persisted under a different locale. Any code path
that goes through the catalog `StructType.getField()` (e.g. `FunctionCallExpr`
in the old analyzer, connector metadata paths) will fail to find the field.
**Suggestion**: Either add the same legacy fallback to
`catalog.StructType.getField()`, or add a `readObject()` that rebuilds
`fieldMap` with `Locale.ROOT` keys after Gson deserialization:
```java
private void readObject(ObjectInputStream in) throws IOException,
ClassNotFoundException {
in.defaultReadObject();
fieldMap.clear();
for (StructField field : fields) {
fieldMap.put(field.getName().toLowerCase(Locale.ROOT), field);
}
}
```
### Issue 2 — `StructElement.getSignatures()` uses raw name for map lookup
(P2)
`StructElement.java:88` looks up the field name directly in `nameToFields`
**without any `toLowerCase()`**:
```java
String name = ((StringLikeLiteral) child(1)).getStringValue();
if (!structArgType.getNameToFields().containsKey(name)) { // raw name vs
lowered keys
```
The map keys are lowered (via `StructField` constructor), so
`struct_element(s, 'FieldA')` looks up `"FieldA"` against key `"fielda"` →
**always misses** for mixed-case names.
This is a pre-existing bug, but the PR's `canonicalizeStructSelector()` in
`ExpressionAnalyzer` only handles `ElementAt`, not `StructElement`. I confirmed
via grep that there is no `visitStructElement` in `ExpressionAnalyzer` and no
rewrite rule that converts `StructElement` to `ElementAt` — so `StructElement`
goes through its own binding path and never gets canonicalized.
**Suggestion**: Add `StructElement` canonicalization in
`ExpressionAnalyzer.visitUnboundFunction()` alongside the existing `ElementAt`
handling, or fix `getSignatures()` to lower the lookup key.
### Issue 3 — `IcebergScanNode.getPathPartitionKeys()` uses
`String::toLowerCase` without `Locale.ROOT` (P2)
```java
// IcebergScanNode.java:385
return icebergTable.spec().fields().stream()
.map(PartitionField::name).map(String::toLowerCase) // ← no Locale.ROOT
.collect(Collectors.toList());
```
This file is not in the PR's changed files. After the PR, struct field names
in the Nereids layer use `toLowerCase(Locale.ROOT)`, but partition key names
from this path still use locale-dependent `toLowerCase()`. On a Turkish JVM,
partition key `"I"` becomes `"ı"` while the corresponding struct field name
becomes `"i"` — they won't match.
### Issue 4 — `catalog.StructType.addField()` key generation may be
inconsistent (P2)
```java
public void addField(StructField field) {
fieldMap.put(field.getName().toLowerCase(), field); // ← no Locale.ROOT
}
```
If the PR changes `getField()` to use `toLowerCase(Locale.ROOT)` but
`addField()` still uses `toLowerCase()` (default locale), then after an `ALTER
TABLE ADD COLUMN` on a struct, the new field's key and the lookup key could
diverge on non-ROOT locales.
### What works correctly
- ✅ Nereids `StructType.getField()` legacy fallback — verified it correctly
resolves Turkish-persisted fields via `equalsIgnoreCase`
- ✅ Pure ASCII field names — `toLowerCase()` and `toLowerCase(Locale.ROOT)`
produce identical results for ASCII, so the vast majority of users are
unaffected
- ✅ `ElementAt` canonicalization in `ExpressionAnalyzer` — covers
`element_at()` and dotted dereference paths
- ✅ `NestedColumnPruning` / `AccessPathExpressionCollector` `Locale.ROOT`
changes — consistent with Nereids struct keys
### Test methodology
I wrote a standalone Java test that simulates:
1. `StructElement` raw name lookup against lowered map keys → confirmed miss
2. Turkish locale field persistence + ROOT locale lookup → confirmed catalog
`getField()` miss
3. Nereids legacy fallback → confirmed it resolves the miss
4. `addField()` mixed-key scenario
5. Gson deserialization with stale `fieldMap` keys → confirmed miss
6. `toLowerCase()` vs `toLowerCase(Locale.ROOT)` divergence for `"I"`,
`"Info"` under Turkish locale
All 4 failure scenarios are specific to **non-ASCII field names + non-ROOT
JVM locale**. For the common case (ASCII + English locale), there are no
compatibility issues.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
