zhuxiangyi commented on code in PR #9423:
URL: https://github.com/apache/paimon/pull/9423#discussion_r3891223269
##########
paimon-format/src/main/java/org/apache/parquet/filter2/predicate/ParquetFilters.java:
##########
@@ -273,9 +278,19 @@ public FilterPredicate visitNotIn(FieldRef fieldRef,
List<Object> literals) {
throw new UnsupportedOperationException();
}
+ /**
+ * A nested field carries no index into the file, only a path, so it
is re-dispatched under
+ * a {@link FieldRef} naming that path. Every other transform - casts,
string functions -
+ * has no column of its own to filter on and is given up here.
+ */
@Override
public FilterPredicate visitNonFieldLeaf(LeafPredicate predicate) {
- throw new UnsupportedOperationException();
+ if (!(predicate.transform() instanceof NestedFieldTransform)) {
+ throw new UnsupportedOperationException();
+ }
+ NestedFieldTransform nested = (NestedFieldTransform)
predicate.transform();
+ FieldRef pathRef = new FieldRef(UNUSED_INDEX, nested.fieldName(),
nested.outputType());
Review Comment:
Thanks, this was exactly right and I could reproduce it.
The path resolution itself was fine — `findFileColumn` walks the components
and returns a `FileColumn` carrying both the resolved path and the physical
type. What I got wrong was one level up: `primitiveType()` discarded the path
and returned only the type, so the decimal and timestamp visitors had nothing
to rebuild the column from except `PrimitiveType.getName()`, which can only be
the leaf's own name. A nested BIGINT happened to be fine because it goes
through `pushdownTarget`, which does keep the path — which is also why my
original tests missed this.
`primitiveType` is now `fileColumn` and returns the whole `FileColumn`;
`decimalColumn` and `timestampColumn` likewise, and the three visitors build
their column from `column.path`.
Tests against actual row groups as you asked — each writes two rows and
asserts the matching row survives the filter:
- `ParquetFormatReadWriteTest.testNestedDecimalPredicateKeepsMatchingRows`
- `ParquetFormatReadWriteTest.testNestedTimestampPredicateKeepsMatchingRows`
-
`ParquetFormatReadWriteTest.testNestedLocalZonedTimestampPredicateKeepsMatchingRows`
- `ParquetFormatReadWriteTest.testNestedBigIntPredicateKeepsMatchingRows`
(control)
One note on how they assert, in case it looks loose: parquet filtering is
row-group granular, so with both rows in one row group a matching predicate
legitimately returns both. The tests assert the matching row is present rather
than that exactly one row comes back, since what we need to catch is the match
disappearing. Happy to tighten this if you would rather see row groups
separated explicitly.
Filter-level coverage: `testNestedDecimalKeepsTheFullPath`,
`testNestedTimestampKeepsTheFullPath`,
`testNestedLocalZonedTimestampKeepsTheFullPath`, and
`testNestedTimestampMicrosKeepsTheFullPath` for the micros literal path.
Writing those turned up two more holes in my own coverage, now closed:
- the decimal visitor builds a column per physical type and I had only
exercised INT64 — `testNestedDecimalKeepsTheFullPathForEveryPhysicalType`
covers INT32, INT64, FIXED_LEN_BYTE_ARRAY and BINARY;
- `IN` / `NOT IN` build the column through the same visitor —
`testNestedDecimalInAndNotInKeepTheFullPath`.
##########
paimon-common/src/main/java/org/apache/paimon/predicate/NestedFieldTransform.java:
##########
@@ -0,0 +1,182 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.predicate;
+
+import org.apache.paimon.data.InternalRow;
+import org.apache.paimon.types.DataType;
+import org.apache.paimon.types.RowType;
+
+import
org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonCreator;
+import
org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonIgnore;
+import
org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonProperty;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+import static org.apache.paimon.utils.InternalRowUtils.get;
+import static org.apache.paimon.utils.Preconditions.checkArgument;
+
+/**
+ * Transform that extracts a field nested inside a row-typed column, for
example {@code addr.city}.
+ *
+ * <p>The transform keeps the enclosing top-level column as its only {@link
#inputs() input}, so
+ * anything that rewrites field indices (schema projection, for instance)
keeps working without
+ * knowing about nesting. The positions below that column are held separately
in {@link #path()}.
+ *
+ * <p>Deliberately <b>not</b> a {@link FieldTransform}: {@link
LeafPredicate#fieldRefOptional()}
+ * returns empty for it, which is what keeps every consumer that equates a
leaf with a top-level
+ * column — min/max pruning, file index lookup, ORC pushdown, schema evolution
— from silently
+ * reading the enclosing column's metadata as if it belonged to the nested
field. Those consumers
+ * give up on this transform instead, which costs pruning but never rows.
+ */
+public class NestedFieldTransform implements Transform {
+
+ private static final long serialVersionUID = 1L;
+
+ public static final String NAME = "NESTED_FIELD_REF";
+
+ public static final String FIELD_FIELD_REF = "fieldRef";
+ public static final String FIELD_PATH = "path";
+
+ /** The top-level row-typed column the nested field lives in. */
+ private final FieldRef fieldRef;
+
+ /** Positions to descend, relative to {@code fieldRef}'s row type. Never
empty. */
+ private final List<Integer> path;
+
+ private final String name;
+ private final DataType outputType;
+
+ @JsonCreator
+ public NestedFieldTransform(
+ @JsonProperty(FIELD_FIELD_REF) FieldRef fieldRef,
+ @JsonProperty(FIELD_PATH) List<Integer> path) {
+ checkArgument(path != null && !path.isEmpty(), "Nested field path must
not be empty.");
+ this.fieldRef = fieldRef;
+ this.path = Collections.unmodifiableList(new ArrayList<>(path));
+
+ StringBuilder nameBuilder = new StringBuilder(fieldRef.name());
+ DataType current = fieldRef.type();
+ for (int position : this.path) {
+ checkArgument(
+ current instanceof RowType,
+ "Nested field path of '%s' descends into a non-row type
%s.",
+ fieldRef.name(),
+ current);
+ RowType rowType = (RowType) current;
+ checkArgument(
+ position >= 0 && position < rowType.getFieldCount(),
+ "Nested field position %s is out of range for %s.",
+ position,
+ rowType);
+
nameBuilder.append('.').append(rowType.getFields().get(position).name());
+ current = rowType.getTypeAt(position);
+ }
+ this.name = nameBuilder.toString();
+ this.outputType = current;
+ }
+
+ @Override
+ public String name() {
+ return NAME;
+ }
+
+ @JsonProperty(FIELD_FIELD_REF)
+ public FieldRef fieldRef() {
+ return fieldRef;
+ }
+
+ @JsonProperty(FIELD_PATH)
+ public List<Integer> path() {
+ return path;
+ }
+
+ /** Dot-separated name from the top-level column down to the nested field,
{@code addr.city}. */
+ @JsonIgnore
+ public String fieldName() {
+ return name;
+ }
+
+ @Override
+ @JsonIgnore
+ public List<Object> inputs() {
+ return Collections.singletonList(fieldRef);
+ }
+
+ @Override
+ @JsonIgnore
+ public DataType outputType() {
+ return outputType;
+ }
+
+ /**
+ * Reads the nested field out of {@code row}, which must match the row
type {@link #fieldRef}
+ * was built against. A null anywhere along the path yields null, matching
SQL semantics for
+ * field access on a null struct.
+ */
+ @Override
+ public Object transform(InternalRow row) {
+ int position = fieldRef.index();
+ if (row.isNullAt(position)) {
+ return null;
+ }
+ RowType currentType = (RowType) fieldRef.type();
+ InternalRow current = row.getRow(position,
currentType.getFieldCount());
+
+ for (int i = 0; i < path.size() - 1; i++) {
+ position = path.get(i);
+ if (current.isNullAt(position)) {
+ return null;
+ }
+ RowType nextType = (RowType) currentType.getTypeAt(position);
+ current = current.getRow(position, nextType.getFieldCount());
+ currentType = nextType;
+ }
+
+ int leaf = path.get(path.size() - 1);
+ return get(current, leaf, currentType.getTypeAt(leaf));
+ }
+
+ @Override
+ public Transform copyWithNewInputs(List<Object> inputs) {
+ checkArgument(inputs.size() == 1);
Review Comment:
Good catch, and I reproduced it: remapping a transform on `info.secret` onto
a pruned `ROW<region>` silently produced `info.region`. Storing a bare position
was my mistake — I had thought about the index moving but not about the row
type itself changing shape.
Took the first option you offered. The path is now the ordered component
names rather than positions, and `copyWithNewInputs` re-resolves them against
the replacement row type, so a pruned-away leaf fails closed and a reordered
row type still addresses the same field. Positions are derived once in the
constructor and used only for evaluation.
On the "while ensuring auth reads the full nested dependencies" part — I
have not done that. Keeping `info.secret` in the read schema when a row filter
references it means touching the projection layer, and I was not sure that
belonged in this PR. What is guaranteed now is that the case fails loudly
instead of resolving elsewhere: the exception propagates out of
`TableQueryAuthResult.remapPredicate` and no caller catches it
(`AbstractDataTableScan:134`). If you would rather have the dependency actually
pulled into the projection, please say so — I am glad to do it here or in a
follow-up, whichever you prefer.
Tests at the auth entry point, since that is the path you were pointing at:
-
`TableQueryAuthResultTest.testNestedRowFilterDoesNotDriftWhenTheLeafIsPruned`
-
`TableQueryAuthResultTest.testNestedRowFilterFollowsTheFieldWhenPositionsShift`
and at the transform level,
`NestedFieldTransformTest.testRemapOntoAPrunedRowTypeDoesNotDrift` /
`testRemapFollowsTheFieldWhenPositionsShift`. The second one is there to keep
me honest: a validation that simply throws would pass the first test but fail
this one, since a reordered row type has to resolve to the original field.
In case it is useful for judging the blast radius, I also checked the other
two `copyWithNewInputs` callers: `PredicateProjectionConverter` and
`PartitionValuePredicateVisitor` both pass `fieldRef.type()` through unchanged
and only remap the top-level index, so neither could drift.
`TableQueryAuthResult` is the one caller that re-derives the type from a
different row type.
--
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]
