voonhous commented on code in PR #20086:
URL: https://github.com/apache/hudi/pull/20086#discussion_r4178137487
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiTableHandle.java:
##########
@@ -248,6 +346,28 @@ public HoodieSchema getTableSchema()
return hudiTableSchema.map(Lazy::get).orElse(null);
}
+ @JsonProperty
+ public Map<String, String> getTableConfig()
+ {
+ return lazyTableConfig.get();
+ }
Review Comment:
`getTableConfig()` is serialized for every handle, so COW and _ro tables
also shop the full **hoodie.properties** map.
`captureCommittedInstants` already skips non-MOR tables, can we skip the
table config too?
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiTableHandle.java:
##########
Review Comment:
MOR handles will always send schema to workers as a string, the workers will
parse it back with `HoodieSchema#parse`.
Using this will check field defaults iirc. before, with column-name casing
off, workers used the resolver's schema object directly. The resolver could
return a log-block schema that was parsed without default checking.
Would it be safer for the worker to use `HoodieSchema.parse(tableSchemaStr,
false)` instead?
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiTableHandle.java:
##########
@@ -151,6 +207,48 @@ public HudiTableHandle(
this.limit = requireNonNull(limit, "limit is null");
this.hudiTableSchema = requireNonNull(hudiTableSchema,
"hudiTableSchema is null");
this.lazyLatestCommitTime = Lazy.lazily(latestCommitTimeSupplier);
+ this.lazyTableConfig = requireNonNull(lazyTableConfig,
"lazyTableConfig is null");
+ this.lazyCommittedInstants = requireNonNull(lazyCommittedInstants,
"lazyCommittedInstants is null");
+ this.lazyFileGroupReaderTableState = Lazy.lazily(() ->
buildFileGroupReaderTableState(basePath, lazyTableConfig.get(),
lazyCommittedInstants.get()));
+ }
+
+ /**
+ * The table properties workers need. The create schema is left out: it
can be as large as the table schema, and
+ * workers read the schema from {@link #getTableSchemaStr()}.
+ */
+ private static Map<String, String> toMap(HoodieTableConfig tableConfig)
+ {
+ Properties properties = tableConfig.getProps();
+ return properties.stringPropertyNames().stream()
+ .filter(key ->
!key.equals(HoodieTableConfig.CREATE_SCHEMA.key()))
+ .collect(toImmutableMap(identity(), properties::getProperty));
+ }
+
+ /**
+ * Only file groups with log files need the committed instants, and only
for tables before version 8, whose log
+ * blocks the reader checks against the timeline.
+ */
+ private static Optional<HudiCommittedInstants>
captureCommittedInstants(HoodieTableMetaClient metaClient, HoodieTableType
tableType, String latestCommitTime)
Review Comment:
Just an FYI, we do not have tests covering v6/v7 mor tables for this part.
None of our tests have an inflight or requested commit without a matching
completed file.
Oerhaps we could add a Trino trests tusing
`PayloadOnlyMergerHudiTablesInitializer` that writes c1, c2, c3 to a v6 table,
delete c2's completed .deltacommit file, then check if the _rt returns a base
value for c2's keys. This proves that the cutoff at latestCommitTime keeps c2
in the inflight set.
--
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]
