a-agmon commented on PR #588: URL: https://github.com/apache/iceberg-rust/pull/588#issuecomment-2323281873
> My main concern is that it seems like you have taken a "soft-fail" approach, where, for unsupported operators or nodes, you return None and proceed by effectively ignoring as-yet-unimplemented operations? I think the whole thing should return a Result rather than an Option, and when an unimplemented aspect of Expr is encountered, we should fail, returning a Err with ErrorKind::Unimplemented. This prevents confusion and unexpected behaviour by users whose datafusion queries succeed but give them unexpected results due to partial filters. Thanks for the review and comment, @sdd ! I Will be happy to update this accordingly (will also learn more carefully the visitor approach in the code base to perhaps refactor this further in the future). I do have one question though. The idea I have followed here is that the conversion process operates in an eager-like mode. It works through the expressions, some of which are not implemented yet, but some of which will not be implemented by design (for example: `Expr::ScalarFunction(ScalarFunction)`), and basically returns a predicate that might not reflect exactly your filters but will filter as much as possible to speed up the query. for example, suppose that we have an expression like: `(A > 1 AND B = func(X)) OR (A < 0)` and we know that `B = func(X)` is not an expression support or can evaluate. So the idea here of soft-fail, is to not fail the entire conversion but to return (A > 1) OR (A < 0), which will still contain unnecessary data but will filter out many files that are not relevant and let you enjoy the metadata-based file pruning. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
