voonhous commented on code in PR #19863:
URL: https://github.com/apache/hudi/pull/19863#discussion_r3954794311
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiPredicates.java:
##########
@@ -29,6 +29,10 @@ public class HudiPredicates
public static HudiPredicates from(TupleDomain<ColumnHandle> predicate)
{
+ if (predicate.isNone()) {
+ return new HudiPredicates(TupleDomain.none(), TupleDomain.none());
Review Comment:
Right on the contract, and it goes one step further: returning `all()` as
the remaining filter would not make the Impact statement true either.
`computeEnforced` reads `predicate.getDomains().get()` immediately after
that `checkArgument`, and `predicate` is `newDomain`, the constraint summary,
which is `none()` in this scenario. `TupleDomain.none()` holds
`Optional.empty()`, so it is a `NoSuchElementException` instead of the
`IllegalArgumentException`. Same death before `getSplits`, different exception.
Iceberg and Delta escape that because their `none()` never comes from the
summary. `IcebergMetadata` says it in the branch itself: "Engine does not pass
none Constraint.summary. It can become none when combined with the expression
and connector's domain knowledge." Their `newDomain` is non-none, so
`computeEnforced` is well formed on both arguments. `HudiPredicates.from` reads
`constraint.getSummary()` directly, so Hudi's only route is a `none()` summary,
which `computeEnforced` is not written to accept from either side.
So there is no connector-side change that makes the guard reachable, and the
honest statement is that it is contract self-consistency rather than a behavior
fix. I have rewritten Impact to say that. The squashed commit message on master
keeps the original wording, so I have noted the edit in the body rather than
quietly swapping it.
Your `DeltaLakeSplitManager` point is right and is in the body now: Hive's
`isNone` check guards the `Constraint` argument in `HivePartitionManager`,
which `HudiSplitManager` never reads, while Delta guards both handle predicates
with `emptySplitSource` exactly as here. Your two comment-accuracy points on
`HudiSplitManager` and `TestHudiSplitSource` both hold; I will fold them into
the next hudi-trino PR rather than open an issue for two comment lines.
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/HudiPredicates.java:
##########
@@ -29,6 +29,10 @@ public class HudiPredicates
public static HudiPredicates from(TupleDomain<ColumnHandle> predicate)
{
+ if (predicate.isNone()) {
+ return new HudiPredicates(TupleDomain.none(), TupleDomain.none());
Review Comment:
Agreed that the hypothetical path described in Impact would still fail at
`computeEnforced`. My intent here is upstream parity and defensive handling of
an otherwise unreachable input, since the planner short-circuits before
`applyFilter` today. I'll narrow the Impact wording to avoid claiming
end-to-end support for passing `none()` through `applyFilter`.
--
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]
