voonhous commented on code in PR #19295:
URL: https://github.com/apache/hudi/pull/19295#discussion_r3674413052
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiUtil.java:
##########
@@ -272,6 +272,12 @@ public static HoodieTableMetaClient buildTableMetaClient(
public static Schema constructSchema(List<String> columnNames,
List<HiveType> columnTypes)
{
+ // A count(*)-style query projects no columns at all;
determineSchemaOrThrowException rejects
+ // an empty column list, so build the empty record schema directly.
+ if (columnNames.isEmpty()) {
+ return SchemaBuilder.record("baseRecord").fields().endRecord();
Review Comment:
Updated the description -- the serializer rework and the `count(*)` fix are
now their own items, and the impact section calls out the new
`HudiAvroSerializer` constructor. The `constructSchema` contract change is gone
entirely (see the serializer thread).
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiUtil.java:
##########
@@ -651,8 +657,13 @@ static LinkedHashSet<String>
mergeRequiredColumnNames(HoodieTableConfig tableCon
// Delete markers and the operation field decide record deletion at
merge time
requiredColumnNames.add(HOODIE_IS_DELETED_FIELD);
requiredColumnNames.add(OPERATION_METADATA_FIELD);
- String deleteKey = tableConfig.getProps().getProperty(DELETE_KEY);
- String deleteMarker =
tableConfig.getProps().getProperty(DELETE_MARKER);
+ // Resolve the delete key/marker the way the file-group reader does
(ConfigUtils.getMergeProps):
+ // table merge properties first -- v9+ tables persist these PREFIXED
(hoodie.record.merge.property.*)
+ // and getTableMergeProperties() strips the prefix and bridges legacy
delete payloads -- falling back
+ // to the raw table props, where pre-prefix tables and reader/write
configs carry the plain keys.
+ Map<String, String> tableMergeProps =
tableConfig.getTableMergeProperties();
Review Comment:
Good catch -- this module's profile being off by default let it slip past
the local build. Now passing `tableConfig.getPayloadClass()`, matching what
`ConfigUtils.getMergeProps` resolves here.
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiUtil.java:
##########
@@ -715,14 +726,19 @@ public static List<HiveColumnHandle>
appendMissingMergeRequiredColumns(
/**
* Builds {@link HiveColumnHandle}s, preserving physical (data-column)
index, for the data columns whose names
* appear in {@code columnNames}. Names that are not data columns (e.g.
Hudi meta fields) or whose types are not
- * supported by the storage format are skipped.
+ * supported by the storage format are skipped. Matching is
case-insensitive: predicted merge columns carry the
+ * table schema's field casing (e.g. the {@code Op} delete key of DMS
tables) while metastore column names are
+ * lowercased.
*/
private static List<HiveColumnHandle> buildColumnHandles(Table table,
TypeManager typeManager, Set<String> columnNames, HiveTimestampPrecision
timestampPrecision)
{
+ Set<String> lowercaseColumnNames = columnNames.stream()
+ .map(name -> name.toLowerCase(Locale.ROOT))
+ .collect(Collectors.toSet());
ImmutableList.Builder<HiveColumnHandle> columns =
ImmutableList.builder();
int hiveColumnIndex = 0;
for (Column field : table.getDataColumns()) {
- if (columnNames.contains(field.getName())) {
+ if
(lowercaseColumnNames.contains(field.getName().toLowerCase(Locale.ROOT))) {
Review Comment:
You're right -- `appendMissingMergeRequiredColumns` recovers it on every
merge path (and the full-schema branch is a superset), so the folding changed
no outcome. Reverted; the javadoc now points at the recovery instead.
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/reader/HudiTrinoReaderContext.java:
##########
@@ -176,9 +184,25 @@ private List<HiveColumnHandle>
buildRequiredColumnHandles(HoodieSchema requiredS
{
List<HiveColumnHandle> handles = new ArrayList<>();
for (HoodieSchemaField field : requiredSchema.getFields()) {
- handles.add(colNameToHandle.computeIfAbsent(
+ HiveColumnHandle handle = colNameToHandle.computeIfAbsent(
field.name().toLowerCase(Locale.ROOT),
- _ -> HudiUtil.toColumnHandle(field)));
+ _ -> HudiUtil.toColumnHandle(field));
+ if (!handle.getName().equals(field.name())) {
Review Comment:
Covered by the serializer change now: parquet resolves columns
case-insensitively and hudi-common reads merge fields off the record's schema,
which `serialize()` stamps with the table casing. Removed.
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/util/HudiAvroSerializer.java:
##########
@@ -112,23 +112,66 @@ public class HudiAvroSerializer
private final List<HiveColumnHandle> columnHandles;
private final List<Type> columnTypes;
private final Schema schema;
+ // serialize() writes page channel i at record position
channelToFieldPosition[i]. -1 marks a
+ // hidden column: a Trino synthesized metadata column ($path, $file_size,
$file_modified_time,
+ // $partition) with no counterpart in the data file or table schema -- its
value is prefilled
+ // per split from PrefilledColumnValues -- so it has no record field and
its channel is skipped.
+ private final int[] channelToFieldPosition;
public HudiAvroSerializer(List<HiveColumnHandle> columnHandles,
PrefilledColumnValues prefilledColumnValues)
{
this.columnHandles = columnHandles;
this.columnTypes =
columnHandles.stream().map(HiveColumnHandle::getType).toList();
- // Fetches projected schema
+ // The record schema carries only columns that exist in the data:
hidden (synthesized
+ // metadata) columns are answered from the split by
PrefilledColumnValues, not read from
+ // the file, and cannot be described as Avro fields
this.schema = constructSchema(columnHandles.stream().filter(ch ->
!ch.isHidden()).map(HiveColumnHandle::getName).toList(),
columnHandles.stream().filter(ch ->
!ch.isHidden()).map(HiveColumnHandle::getHiveType).toList());
this.prefilledColumnValues = prefilledColumnValues;
+ // With hidden columns excluded from the schema, the remaining
channels map to sequential
+ // field positions; hidden channels map to -1
+ int[] mapping = new int[columnHandles.size()];
Review Comment:
Went with fixing it at the source -- the two-arg constructor no longer
builds a schema (`serialize()` requires the schema-carrying one), and the
`constructSchema` special case is reverted. The empty projection stays pinned
by the bare `count(*)` asserts in `TestHudiMorMergeModeSemantics`.
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/reader/HudiTrinoReaderContext.java:
##########
@@ -243,9 +267,8 @@ protected Option<HoodieRecordMerger>
getRecordMerger(RecordMergeMode mergeMode,
// Dispatch on the table's merge mode, mirroring
HoodieAvroReaderContext. The Trino reader
// operates on IndexedRecord, so the Avro mergers apply directly.
Using the read-time merger
// (combineAndGetUpdateValue) rather than a fixed preCombine merger
keeps COMMIT_TIME_ORDERING
- // and custom-payload tables correct on MoR reads.
- // TODO(apache/hudi#18898): add MoR read tests for delete markers and
custom payloads to
- // exercise the EVENT_TIME_ORDERING (combineAndGetUpdateValue) and
CUSTOM branches below.
+ // and custom-payload tables correct on MoR reads;
TestHudiMorMergeModeSemantics and
+ // TestHudiMorPayloadSemantics cover these branches end-to-end
(apache/hudi#18898).
Review Comment:
Narrowed the claim to the CUSTOM arm. Didn't reopen the TODO though:
`BufferedRecordMergerFactory` picks the buffered merger from the mode alone and
never dereferences the ordering arms' return values (only
`getMergingStrategy()`), so there's nothing those arms could be tested against
-- the comment now says exactly that.
--
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]
