leaves12138 commented on code in PR #8782:
URL: https://github.com/apache/paimon/pull/8782#discussion_r3662323380
##########
paimon-core/src/main/java/org/apache/paimon/utils/FormatReaderMapping.java:
##########
@@ -332,6 +347,66 @@ private List<DataField> readDataFields(
return readDataFields;
}
+ private DataField selectedKeysDataField(DataField expectedField,
DataField dataField) {
+ RowType selectedKeysType = (RowType) expectedField.type();
+ DataType dataValueType = ((MapType)
dataField.type()).getValueType();
+ List<DataField> selectedKeysDataFields = new ArrayList<>();
+ for (DataField selectedKeyField : selectedKeysType.getFields()) {
+ selectedKeysDataFields.add(
+ selectedKeyField.newType(
+
dataValueType.copy(selectedKeyField.type().isNullable())));
+ }
+ return dataField
+ .newType(selectedKeysType.copy(selectedKeysDataFields))
+ .newDescription(expectedField.description());
+ }
+
+ private void checkSelectedKeysDataField(DataField dataField) {
+ checkArgument(
+ dataField.type() instanceof MapType,
+ "Selected-key MAP field %s should be MAP type in data
schema.",
+ dataField.name());
+ }
+
+ private Set<Integer> selectedKeysFieldIds(
+ TableSchema tableSchema, List<DataField> expectedFields) {
+ CoreOptions options = CoreOptions.fromMap(tableSchema.options());
+ Map<Integer, DataField> tableFields = tableSchema.idToFieldMap();
+ Set<Integer> selectedKeysFieldIds = new HashSet<>();
+ for (DataField expectedField : expectedFields) {
+ if
(MapSelectedKeysMetadataUtils.isMapSelectedKeysField(expectedField)) {
Review Comment:
Could we avoid classifying an ordinary ROW column as an internal
selected-key request solely from its user-visible description? `expectedFields`
normally retain the table column comments, so a valid column such as `payload
ROW<...> COMMENT '__PAIMON_MAP_SELECTED_KEYS:key1'` enters this branch, then
fails below because the corresponding `tableField` is ROW rather than MAP. This
makes reads fail for a table unrelated to MAP shared shredding. Please include
the original table field type in the discriminator (for example, only recognize
the marker when the same field id is a MAP in `tableSchema`) and add a
regression test for a normal ROW column whose comment starts with this prefix.
##########
paimon-spark/paimon-spark-common/src/main/scala/org/apache/paimon/spark/catalyst/optimizer/PushDownMapSelectedKeys.scala:
##########
@@ -187,18 +189,27 @@ abstract class PushDownMapSelectedKeysBase extends
Rule[LogicalPlan] {
return false
}
+ val options = CoreOptions.fromMap(scan.table.options())
access.mapType match {
case MapType(StringType, _, _) =>
fieldType(scan.table.rowType(), access.fieldName) match {
case Some(mapType: org.apache.paimon.types.MapType) if
isStringKeyMap(mapType) =>
-
CoreOptions.fromMap(scan.table.options()).mapStorageLayout(access.fieldName) ==
- MapStorageLayout.SHARED_SHREDDING
+ options.mapStorageLayout(access.fieldName) ==
MapStorageLayout.SHARED_SHREDDING &&
+ !usesMapMergeAggregator(options, access.fieldName)
case _ => false
}
case _ => false
}
}
+ private def usesMapMergeAggregator(options: CoreOptions, fieldName: String):
Boolean = {
Review Comment:
This denylist only protects the two built-in MAP aggregators, but
`FieldAggregatorFactory` is extensible through `ServiceLoader`. A valid custom
MAP aggregator (for example `my_merge_map`) is not recognized here, so the
pushdown still changes its input type from MAP to ROW and the custom factory
can fail or silently apply different semantics. Could we conservatively skip
selected-key pushdown for any configured field/default aggregator unless it is
explicitly known to be projection-safe, rather than denylisting known MAP
aggregator names? A regression test with a custom MAP-only aggregator would
cover this extension path.
--
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]
