amogh-jahagirdar commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2790420412
##########
core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java:
##########
@@ -43,8 +49,47 @@ public List<String> select() {
return select;
}
+ /**
+ * Returns the filter expression, deserializing it without schema context.
+ *
+ * <p>Note: This method does not perform type-aware deserialization and may
not work correctly for
+ * BINARY, FIXED, and DECIMAL types. Use {@link #filter(Schema)} instead for
proper type handling.
+ *
+ * @return the filter expression, or null if no filter was specified
+ * @deprecated since 1.11.0, will be removed in 1.12.0; use {@link
#filter(Schema)} instead for
+ * proper type-aware deserialization
+ */
+ @Deprecated
Review Comment:
I'm not sure about deprecating this. In the long run we do expect the
expression filter to be self-describing with the grammar that's being proposed
by @rdblue ; in particular we'd expect a data type at that point and for
ExpressionParser.fromJson(json) to just work for that case.
So in the long run, I'd actually expect `filter(Schema)` to be deprecated,
we just need it for now due to limitations in the protocol.
I think we should probably just keep both (with the limitation that filter()
won't work for the 3 specific data types) and then drop the filter(Schema)
if/when that proposal materializes.
Unless there are other cases we can envision where we'd want to pass a
schema here?
##########
core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java:
##########
@@ -160,8 +212,38 @@ public Builder withSelect(List<String> projection) {
return this;
}
+ /**
+ * Sets the filter expression for the scan.
+ *
+ * @param expression the filter expression
+ * @return this builder
+ * @deprecated since 1.11.0, will be removed in 1.12.0; this method
serializes the expression to
+ * JSON immediately, which may lose type information for BINARY,
FIXED, and DECIMAL types
+ */
+ @Deprecated
Review Comment:
Same rationale as above, I'm not sure we should deprecate this because I
think in the long run we'd expect such an API to exist. We can avoid
API/deprecation churn by just keeping both until any grammar/protocol changes
are made.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
