-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[spark] Support nested fields in SparkFilterConverter #8399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
8c160cf
1430128
78eb7c4
179122c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,10 @@ | |
| import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonCreator; | ||
| import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonProperty; | ||
|
|
||
| import javax.annotation.Nullable; | ||
|
|
||
| import java.io.Serializable; | ||
| import java.util.Arrays; | ||
| import java.util.Objects; | ||
|
|
||
| /** A reference to a field in an input. */ | ||
|
|
@@ -34,19 +37,31 @@ public class FieldRef implements Serializable { | |
| private static final String FIELD_INDEX = "index"; | ||
| private static final String FIELD_NAME = "name"; | ||
| private static final String FIELD_TYPE = "type"; | ||
| private static final String FIELD_NESTED_INDEXES = "nestedIndexes"; | ||
| private static final String FIELD_NESTED_ARITIES = "nestedArities"; | ||
|
|
||
| private final int index; | ||
| private final String name; | ||
| private final DataType type; | ||
| @Nullable private final int[] nestedIndexes; | ||
| @Nullable private final int[] nestedArities; | ||
|
|
||
| public FieldRef(int index, String name, DataType type) { | ||
| this(index, name, type, null, null); | ||
| } | ||
|
|
||
| @JsonCreator | ||
| public FieldRef( | ||
| @JsonProperty(FIELD_INDEX) int index, | ||
| @JsonProperty(FIELD_NAME) String name, | ||
| @JsonProperty(FIELD_TYPE) DataType type) { | ||
| @JsonProperty(FIELD_TYPE) DataType type, | ||
| @JsonProperty(FIELD_NESTED_INDEXES) @Nullable int[] nestedIndexes, | ||
| @JsonProperty(FIELD_NESTED_ARITIES) @Nullable int[] nestedArities) { | ||
| this.index = index; | ||
| this.name = name; | ||
| this.type = type; | ||
| this.nestedIndexes = nestedIndexes; | ||
| this.nestedArities = nestedArities; | ||
| } | ||
|
|
||
| @JsonProperty(FIELD_INDEX) | ||
|
|
@@ -64,6 +79,38 @@ public DataType type() { | |
| return type; | ||
| } | ||
|
|
||
| @JsonProperty(FIELD_NESTED_INDEXES) | ||
| @Nullable | ||
| public int[] nestedIndexes() { | ||
| return nestedIndexes; | ||
| } | ||
|
|
||
| @JsonProperty(FIELD_NESTED_ARITIES) | ||
| @Nullable | ||
| public int[] nestedArities() { | ||
| return nestedArities; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a copy of this ref with the given top-level index, preserving name, type and nested | ||
| * path metadata. | ||
| */ | ||
| public FieldRef withIndex(int newIndex) { | ||
| return new FieldRef(newIndex, name, type, nestedIndexes, nestedArities); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the name of the top-level field this ref points at: {@link #name()} for a top-level | ||
| * ref, the first path segment for a nested ref (whose name is the full dotted path). | ||
| */ | ||
| public String topLevelName() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new helper also needs to be reflected in the field-name collection paths. |
||
| if (nestedIndexes == null || nestedIndexes.length == 0) { | ||
| return name; | ||
| } | ||
| int firstDot = name.indexOf('.'); | ||
| return firstDot < 0 ? name : name.substring(0, firstDot); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
|
|
@@ -75,12 +122,17 @@ public boolean equals(Object o) { | |
| FieldRef fieldRef = (FieldRef) o; | ||
| return index == fieldRef.index | ||
| && Objects.equals(name, fieldRef.name) | ||
| && Objects.equals(type, fieldRef.type); | ||
| && Objects.equals(type, fieldRef.type) | ||
| && Arrays.equals(nestedIndexes, fieldRef.nestedIndexes) | ||
| && Arrays.equals(nestedArities, fieldRef.nestedArities); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(index, name, type); | ||
| int result = Objects.hash(index, name, type); | ||
| result = 31 * result + Arrays.hashCode(nestedIndexes); | ||
| result = 31 * result + Arrays.hashCode(nestedArities); | ||
| return result; | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /* | ||
| * 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.types.DataTypes; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** Tests for {@link FieldRef}. */ | ||
| class FieldRefTest { | ||
|
|
||
| @Test | ||
| void testTopLevelName() { | ||
| FieldRef topLevel = new FieldRef(0, "a.b", DataTypes.INT()); | ||
| assertThat(topLevel.topLevelName()).isEqualTo("a.b"); | ||
|
|
||
| FieldRef nested = new FieldRef(0, "a.b", DataTypes.INT(), new int[] {1}, new int[] {2}); | ||
| assertThat(nested.topLevelName()).isEqualTo("a"); | ||
|
|
||
| FieldRef deepNested = | ||
| new FieldRef(0, "a.b.c", DataTypes.INT(), new int[] {1, 0}, new int[] {2, 3}); | ||
| assertThat(deepNested.topLevelName()).isEqualTo("a"); | ||
| } | ||
|
|
||
| @Test | ||
| void testWithIndexPreservesNestedMetadata() { | ||
| FieldRef nested = new FieldRef(3, "a.b", DataTypes.INT(), new int[] {1}, new int[] {2}); | ||
| FieldRef remapped = nested.withIndex(0); | ||
| assertThat(remapped.index()).isEqualTo(0); | ||
| assertThat(remapped.name()).isEqualTo("a.b"); | ||
| assertThat(remapped.type()).isEqualTo(DataTypes.INT()); | ||
| assertThat(remapped.nestedIndexes()).containsExactly(1); | ||
| assertThat(remapped.nestedArities()).containsExactly(2); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,16 +187,15 @@ private static Map<Integer, Transform> transformRemapping( | |
| for (Object input : transform.inputs()) { | ||
| if (input instanceof FieldRef) { | ||
| FieldRef ref = (FieldRef) input; | ||
| int newIndex = outputRowType.getFieldIndex(ref.name()); | ||
| int newIndex = outputRowType.getFieldIndex(ref.topLevelName()); | ||
| if (newIndex < 0) { | ||
| throw new IllegalArgumentException( | ||
| "Column masking refers to field '" | ||
| + ref.name() | ||
| + "' which is not present in output row type " | ||
| + outputRowType); | ||
| } | ||
| DataType type = outputRowType.getTypeAt(newIndex); | ||
| newInputs.add(new FieldRef(newIndex, ref.name(), type)); | ||
| newInputs.add(ref.withIndex(newIndex)); | ||
| } else { | ||
| newInputs.add(input); | ||
| } | ||
|
|
@@ -221,16 +220,14 @@ public Predicate visit(LeafPredicate predicate) { | |
| for (Object input : transform.inputs()) { | ||
| if (input instanceof FieldRef) { | ||
| FieldRef ref = (FieldRef) input; | ||
| String fieldName = ref.name(); | ||
| int newIndex = outputRowType.getFieldIndex(fieldName); | ||
| int newIndex = outputRowType.getFieldIndex(ref.topLevelName()); | ||
| if (newIndex < 0) { | ||
| throw new RuntimeException( | ||
| String.format( | ||
| "Unable to read data without column %s when row filter enabled.", | ||
| fieldName)); | ||
| ref.name())); | ||
| } | ||
| DataType type = outputRowType.getTypeAt(newIndex); | ||
| newInputs.add(new FieldRef(newIndex, fieldName, type)); | ||
| newInputs.add(ref.withIndex(newIndex)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still derives
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks sir. for your reviews improved me |
||
| } else { | ||
| newInputs.add(input); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| /* | ||
| * 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.catalog; | ||
|
|
||
| import org.apache.paimon.data.BinaryString; | ||
| import org.apache.paimon.data.GenericRow; | ||
| import org.apache.paimon.data.InternalRow; | ||
| import org.apache.paimon.predicate.Equal; | ||
| import org.apache.paimon.predicate.FieldRef; | ||
| import org.apache.paimon.predicate.FieldTransform; | ||
| import org.apache.paimon.predicate.LeafPredicate; | ||
| import org.apache.paimon.predicate.Predicate; | ||
| import org.apache.paimon.predicate.Transform; | ||
| import org.apache.paimon.types.DataType; | ||
| import org.apache.paimon.types.DataTypes; | ||
| import org.apache.paimon.types.RowType; | ||
| import org.apache.paimon.utils.IteratorRecordReader; | ||
| import org.apache.paimon.utils.JsonSerdeUtil; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** Tests for {@link TableQueryAuthResult}. */ | ||
| class TableQueryAuthResultTest { | ||
|
|
||
| private static final RowType NESTED_TYPE = | ||
| RowType.of( | ||
| new DataType[] {DataTypes.INT(), DataTypes.STRING()}, new String[] {"b", "c"}); | ||
|
|
||
| /** Output row type with top-level fields reordered relative to the table schema (id, s). */ | ||
| private static final RowType OUTPUT_TYPE = | ||
| RowType.of(new DataType[] {NESTED_TYPE, DataTypes.INT()}, new String[] {"s", "id"}); | ||
|
|
||
| private static Transform nestedFieldTransform() { | ||
| return new FieldTransform( | ||
| new FieldRef(1, "s.b", DataTypes.INT(), new int[] {0}, new int[] {2})); | ||
| } | ||
|
|
||
| @Test | ||
| void testNestedRowFilterRemappedByTopLevelField() throws Exception { | ||
| Predicate predicate = | ||
| LeafPredicate.of( | ||
| nestedFieldTransform(), Equal.INSTANCE, Collections.singletonList(10)); | ||
| TableQueryAuthResult authResult = | ||
| new TableQueryAuthResult( | ||
| Collections.singletonList(JsonSerdeUtil.toJson(predicate)), null); | ||
|
|
||
| List<InternalRow> rows = | ||
| Arrays.asList( | ||
| GenericRow.of(GenericRow.of(10, BinaryString.fromString("x")), 1), | ||
| GenericRow.of(GenericRow.of(20, BinaryString.fromString("y")), 2)); | ||
|
|
||
| List<Integer> ids = new ArrayList<>(); | ||
| authResult | ||
| .doAuth(new IteratorRecordReader<>(rows.iterator()), OUTPUT_TYPE) | ||
| .forEachRemaining(row -> ids.add(row.getInt(1))); | ||
|
|
||
| assertThat(ids).containsExactly(1); | ||
| } | ||
|
|
||
| @Test | ||
| void testNestedColumnMaskingRemappedByTopLevelField() throws Exception { | ||
| Map<String, String> masking = | ||
| Collections.singletonMap("id", JsonSerdeUtil.toJson(nestedFieldTransform())); | ||
| TableQueryAuthResult authResult = new TableQueryAuthResult(null, masking); | ||
|
|
||
| List<InternalRow> rows = | ||
| Collections.singletonList( | ||
| GenericRow.of(GenericRow.of(42, BinaryString.fromString("x")), 1)); | ||
|
|
||
| List<Integer> ids = new ArrayList<>(); | ||
| authResult | ||
| .doAuth(new IteratorRecordReader<>(rows.iterator()), OUTPUT_TYPE) | ||
| .forEachRemaining(row -> ids.add(row.getInt(1))); | ||
|
|
||
| assertThat(ids).containsExactly(42); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
FieldRefnow carriesnestedIndexes/nestedArities, every place that remaps aFieldRefhas to preserve them. Existing remappers such asPredicateProjectionConverter,PartitionValuePredicateVisitor, andTableQueryAuthResultstill rebuild refs with the 3-arg constructor, so a pushed predicate likea.b = 1loses its nested path after projection/auth/partition remapping. The resultingFieldTransformthen reads top-levelaas the leaf type instead of traversing tob, which can produce wrong filtering or runtime failures. Please add a copy/remap helper that preserves the nested metadata and use it for allFieldRefrewrites before enabling nested predicates.