You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "singhpk234 (via GitHub)" <gi...@apache.org> on 2025/12/18 11:59:16 UTC
[PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
singhpk234 opened a new pull request, #14882:
URL: https://github.com/apache/iceberg/pull/14882
### About the change
Fix is present mostly in commit [Spark: Handle complex type in expression in RemoteScanPlanning](https://github.com/apache/iceberg/commit/881b1185f15d3189199a3a1b0cc6bee22ec3d2b9) need to sleep over this to think this through, presently i just made the existing tests pass with following this https://github.com/apache/iceberg/issues/14879#issuecomment-3669516733.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Spark: Handle binary, fixed types in expression in RemoteScanPlanning [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2799571618
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -587,6 +588,32 @@ public <T> Literal<T> to(Type type) {
BigDecimal decimal = new BigDecimal(value().toString());
return (Literal<T>) new DecimalLiteral(decimal);
+ case FIXED:
+ try {
+ ByteBuffer buffer =
+ ByteBuffer.wrap(
+ BaseEncoding.base16().decode(value().toString().toUpperCase(Locale.ROOT)));
Review Comment:
minor: May just have a constant for `BaseEncoding.base16()`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2630780528
##########
core/src/main/java/org/apache/iceberg/SingleValueParser.java:
##########
@@ -119,40 +119,60 @@ public static Object fromJson(Type type, JsonNode defaultValue) {
}
return uuid;
case DATE:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- return DateTimeUtil.isoDateToDays(defaultValue.textValue());
+ if (defaultValue.isTextual()) {
+ return DateTimeUtil.isoDateToDays(defaultValue.textValue());
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToInt()) {
+ return defaultValue.intValue();
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
+ }
case TIME:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- return DateTimeUtil.isoTimeToMicros(defaultValue.textValue());
+ if (defaultValue.isTextual()) {
+ return DateTimeUtil.isoTimeToMicros(defaultValue.textValue());
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
+ }
case TIMESTAMP:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- if (((Types.TimestampType) type).shouldAdjustToUTC()) {
- String timestampTz = defaultValue.textValue();
- Preconditions.checkArgument(
- DateTimeUtil.isUTCTimestamptz(timestampTz),
- "Cannot parse default as a %s value: %s, offset must be +00:00",
- type,
- defaultValue);
- return DateTimeUtil.isoTimestamptzToMicros(timestampTz);
+ if (defaultValue.isTextual()) {
+ if (((Types.TimestampType) type).shouldAdjustToUTC()) {
+ String timestampTz = defaultValue.textValue();
+ Preconditions.checkArgument(
+ DateTimeUtil.isUTCTimestamptz(timestampTz),
+ "Cannot parse default as a %s value: %s, offset must be +00:00",
+ type,
+ defaultValue);
+ return DateTimeUtil.isoTimestamptzToMicros(timestampTz);
+ } else {
+ return DateTimeUtil.isoTimestampToMicros(defaultValue.textValue());
+ }
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
} else {
- return DateTimeUtil.isoTimestampToMicros(defaultValue.textValue());
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
}
case TIMESTAMP_NANO:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- if (((Types.TimestampNanoType) type).shouldAdjustToUTC()) {
- String timestampTzNano = defaultValue.textValue();
- Preconditions.checkArgument(
- DateTimeUtil.isUTCTimestamptz(timestampTzNano),
- "Cannot parse default as a %s value: %s, offset must be +00:00",
- type,
- defaultValue);
- return DateTimeUtil.isoTimestamptzToNanos(timestampTzNano);
+ if (defaultValue.isTextual()) {
+ if (((Types.TimestampNanoType) type).shouldAdjustToUTC()) {
+ String timestampTzNano = defaultValue.textValue();
+ Preconditions.checkArgument(
+ DateTimeUtil.isUTCTimestamptz(timestampTzNano),
+ "Cannot parse default as a %s value: %s, offset must be +00:00",
+ type,
+ defaultValue);
+ return DateTimeUtil.isoTimestamptzToNanos(timestampTzNano);
+ } else {
+ return DateTimeUtil.isoTimestampToNanos(defaultValue.textValue());
+ }
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
Review Comment:
this is mostly required now since we are now binding with schema, need to think this through
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2635150059
##########
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
public Expression filter() {
- return filter;
+ if (filterJson == null) {
+ return null;
+ }
+ return ExpressionParser.fromJson(filterJson);
+ }
+
+ /**
+ * Returns the filter expression, deserializing it with the provided schema for type inference.
+ *
+ * <p>This method should be preferred over {@link #filter()} as it properly handles BINARY, FIXED,
+ * and DECIMAL types by using schema information for type-aware deserialization.
+ *
+ * @param schema the table schema to use for type-aware deserialization of filter values
+ * @return the filter expression, or null if no filter was specified
+ */
+ public Expression filter(Schema schema) {
Review Comment:
<img width="847" height="333" alt="image" src="https://github.com/user-attachments/assets/f209f4e0-23c6-403e-8c1e-f1903f448ffb" />
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2640614693
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -773,18 +774,20 @@ static void clearPlanningState() {
*
* @param scan the scan to configure
* @param request the plan table scan request containing filters and projections
+ * @param schema the table schema to use for type-aware filter deserialization
* @param <T> the specific scan type (TableScan, IncrementalAppendScan, etc.)
* @return the configured scan with filters and projections applied
*/
private static <T extends Scan<T, FileScanTask, ?>> T configureScan(
- T scan, PlanTableScanRequest request) {
+ T scan, PlanTableScanRequest request, Schema schema) {
T configuredScan = scan;
if (request.select() != null) {
configuredScan = configuredScan.select(request.select());
}
- if (request.filter() != null) {
- configuredScan = configuredScan.filter(request.filter());
+ Expression filter = request.filter(schema);
Review Comment:
i removed the .filter() check because we are marking that as deprecated and if we do this check this way i.e first try deserialize (if its null we bail out early) and based on the nullbability that we feed it to our scan down. Please let me know wdyt considering above.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2790375396
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -775,18 +776,20 @@ static void clearPlanningState() {
*
* @param scan the scan to configure
* @param request the plan table scan request containing filters and projections
+ * @param schema the table schema to use for type-aware filter deserialization
* @param <T> the specific scan type (TableScan, IncrementalAppendScan, etc.)
* @return the configured scan with filters and projections applied
*/
private static <T extends Scan<T, FileScanTask, ?>> T configureScan(
- T scan, PlanTableScanRequest request) {
+ T scan, PlanTableScanRequest request, Schema schema) {
Review Comment:
you mean use scan.schema() ? because configured scan post select would cause issue if I have filter on column not being projected ? I think we can do that, let me refactor
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle binary, fixed types in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#issuecomment-3881968686
Update: I synced with Amogh offline on this, thanks a tons for brainstorming with me and suggesting this alternative in this first place, we bind any ways during the planning in the later all this lacked was a convertor case handling, i added this
No spec change, no messy handling requireed for this particular problem, I added additional test for fixed Type for this
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2788799700
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -775,18 +776,20 @@ static void clearPlanningState() {
*
* @param scan the scan to configure
* @param request the plan table scan request containing filters and projections
+ * @param schema the table schema to use for type-aware filter deserialization
* @param <T> the specific scan type (TableScan, IncrementalAppendScan, etc.)
* @return the configured scan with filters and projections applied
*/
private static <T extends Scan<T, FileScanTask, ?>> T configureScan(
- T scan, PlanTableScanRequest request) {
+ T scan, PlanTableScanRequest request, Schema schema) {
Review Comment:
The schema to use would always be the configured schema for the scan no? In that case I think we can get rid of the 3rd argument and just do request.filter(configuredScan.schema()) below
##########
core/src/main/java/org/apache/iceberg/SingleValueParser.java:
##########
@@ -119,41 +119,14 @@ public static Object fromJson(Type type, JsonNode defaultValue) {
}
return uuid;
case DATE:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- return DateTimeUtil.isoDateToDays(defaultValue.textValue());
+ return parseDateValue(type, defaultValue);
case TIME:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- return DateTimeUtil.isoTimeToMicros(defaultValue.textValue());
+ return parseTimeValue(type, defaultValue);
case TIMESTAMP:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- if (((Types.TimestampType) type).shouldAdjustToUTC()) {
- String timestampTz = defaultValue.textValue();
- Preconditions.checkArgument(
Review Comment:
These are the same as before right? Just a pure refactoring? I'd prefer to keep it as is just to minimize the diff, but not a hard opinion.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle binary, fixed types in expression in RemoteScanPlanning [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2791784301
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -587,6 +588,32 @@ public <T> Literal<T> to(Type type) {
BigDecimal decimal = new BigDecimal(value().toString());
return (Literal<T>) new DecimalLiteral(decimal);
+ case FIXED:
Review Comment:
this is great. I think it would be good to have a unit test in the `api` module for this change
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2635142166
##########
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
public Expression filter() {
- return filter;
+ if (filterJson == null) {
+ return null;
+ }
+ return ExpressionParser.fromJson(filterJson);
+ }
+
+ /**
+ * Returns the filter expression, deserializing it with the provided schema for type inference.
+ *
+ * <p>This method should be preferred over {@link #filter()} as it properly handles BINARY, FIXED,
+ * and DECIMAL types by using schema information for type-aware deserialization.
+ *
+ * @param schema the table schema to use for type-aware deserialization of filter values
+ * @return the filter expression, or null if no filter was specified
+ */
+ public Expression filter(Schema schema) {
Review Comment:
let me think about this a bit more. I also think we have a few more cases across the codebase where we also ser/de `Expression` without a `Schema` and theoretically we would have the same issue in those places as well.
Whatever approach we pick, we'd want to follow up in those other places too
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2635187066
##########
core/src/main/java/org/apache/iceberg/SingleValueParser.java:
##########
@@ -414,4 +387,68 @@ public static void toJson(Type type, Object defaultValue, JsonGenerator generato
throw new UnsupportedOperationException(String.format("Type: %s is not supported", type));
}
}
+
+ private static Object parseDateValue(Type type, JsonNode value) {
+ if (value.isTextual()) {
+ return DateTimeUtil.isoDateToDays(value.textValue());
+ } else if (value.isIntegralNumber() && value.canConvertToInt()) {
Review Comment:
I think these changes are probably not required to fix the underlying issue, so we might want to separate them out and test them individually
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2635142166
##########
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
public Expression filter() {
- return filter;
+ if (filterJson == null) {
+ return null;
+ }
+ return ExpressionParser.fromJson(filterJson);
+ }
+
+ /**
+ * Returns the filter expression, deserializing it with the provided schema for type inference.
+ *
+ * <p>This method should be preferred over {@link #filter()} as it properly handles BINARY, FIXED,
+ * and DECIMAL types by using schema information for type-aware deserialization.
+ *
+ * @param schema the table schema to use for type-aware deserialization of filter values
+ * @return the filter expression, or null if no filter was specified
+ */
+ public Expression filter(Schema schema) {
Review Comment:
let me think about this a bit more. I also think we have a few more cases across the codebase where we also ser/de `Expression` without a `Schema` and theoretically we would have the same issue in those places as well
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2630774273
##########
core/src/main/java/org/apache/iceberg/rest/RESTFileScanTaskParser.java:
##########
@@ -96,13 +97,12 @@ public static FileScanTask fromJson(
Expression filter = null;
if (jsonNode.has(RESIDUAL_FILTER)) {
- filter = ExpressionParser.fromJson(jsonNode.get(RESIDUAL_FILTER));
+ filter = ExpressionParser.fromJson(jsonNode.get(RESIDUAL_FILTER), spec.schema());
Review Comment:
this was caught during the execution phase of spark, need to pass schema for residual
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2640623956
##########
core/src/main/java/org/apache/iceberg/SingleValueParser.java:
##########
@@ -414,4 +387,68 @@ public static void toJson(Type type, Object defaultValue, JsonGenerator generato
throw new UnsupportedOperationException(String.format("Type: %s is not supported", type));
}
}
+
+ private static Object parseDateValue(Type type, JsonNode value) {
+ if (value.isTextual()) {
+ return DateTimeUtil.isoDateToDays(value.textValue());
+ } else if (value.isIntegralNumber() && value.canConvertToInt()) {
Review Comment:
when we serialize the expression the date types are converted to the integer. hence during deserialization we need we would need this to convert int to date, hence i added it here. please let me know if i am missing something here ?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
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 in the literal 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?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2635132264
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -773,18 +774,20 @@ static void clearPlanningState() {
*
* @param scan the scan to configure
* @param request the plan table scan request containing filters and projections
+ * @param schema the table schema to use for type-aware filter deserialization
* @param <T> the specific scan type (TableScan, IncrementalAppendScan, etc.)
* @return the configured scan with filters and projections applied
*/
private static <T extends Scan<T, FileScanTask, ?>> T configureScan(
- T scan, PlanTableScanRequest request) {
+ T scan, PlanTableScanRequest request, Schema schema) {
T configuredScan = scan;
if (request.select() != null) {
configuredScan = configuredScan.select(request.select());
}
- if (request.filter() != null) {
- configuredScan = configuredScan.filter(request.filter());
+ Expression filter = request.filter(schema);
Review Comment:
nit: this change is probably not needed
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2635158004
##########
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
public Expression filter() {
- return filter;
+ if (filterJson == null) {
+ return null;
+ }
+ return ExpressionParser.fromJson(filterJson);
+ }
+
+ /**
+ * Returns the filter expression, deserializing it with the provided schema for type inference.
+ *
+ * <p>This method should be preferred over {@link #filter()} as it properly handles BINARY, FIXED,
+ * and DECIMAL types by using schema information for type-aware deserialization.
+ *
+ * @param schema the table schema to use for type-aware deserialization of filter values
+ * @return the filter expression, or null if no filter was specified
+ */
+ public Expression filter(Schema schema) {
Review Comment:
the other thing we might need to consider is how we would be lazily binding this in other client implementations. @Fokko does pyiceberg have examples of how it does a late-binding similar to this one?
The issue that we have here is that we deserialize an `Expression` where we can only correctly do so when we bind it to a `Schema`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle binary, fixed, and decimal types in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2790461268
##########
core/src/main/java/org/apache/iceberg/SingleValueParser.java:
##########
@@ -414,4 +387,68 @@ public static void toJson(Type type, Object defaultValue, JsonGenerator generato
throw new UnsupportedOperationException(String.format("Type: %s is not supported", type));
}
}
+
+ private static Object parseDateValue(Type type, JsonNode value) {
+ if (value.isTextual()) {
+ return DateTimeUtil.isoDateToDays(value.textValue());
+ } else if (value.isIntegralNumber() && value.canConvertToInt()) {
Review Comment:
my bad on this one i just read : https://iceberg.apache.org/spec/#json-single-value-serialization
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle binary, fixed types in expression in RemoteScanPlanning [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2791786438
##########
spark/v4.1/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoteScanPlanning.java:
##########
@@ -48,11 +46,4 @@ protected static Object[][] parameters() {
}
};
}
-
- @TestTemplate
- @Disabled(
- "binary filter that is used by Spark is not working because ExpressionParser.fromJSON doesn't have the Schema to properly parse the filter expression")
Review Comment:
you might want to remove this for the other Spark versions as well in this PR, since the change is very small
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Spark: Support StringLiteral to Fixed and StringLiteral to Binary Conversions [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #14882:
URL: https://github.com/apache/iceberg/pull/14882
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2630775700
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -657,7 +658,7 @@ public static PlanTableScanResponse planTableScan(
.fromSnapshotInclusive(request.startSnapshotId())
.toSnapshot(request.endSnapshotId());
- configuredScan = configureScan(incrementalScan, request);
+ configuredScan = configureScan(incrementalScan, request, table.schema());
Review Comment:
need to use snapshot schema here
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark: Handle complex type in expression in RemoteScanPlanning [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14882:
URL: https://github.com/apache/iceberg/pull/14882#discussion_r2630827854
##########
core/src/main/java/org/apache/iceberg/SingleValueParser.java:
##########
@@ -119,40 +119,60 @@ public static Object fromJson(Type type, JsonNode defaultValue) {
}
return uuid;
case DATE:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- return DateTimeUtil.isoDateToDays(defaultValue.textValue());
+ if (defaultValue.isTextual()) {
+ return DateTimeUtil.isoDateToDays(defaultValue.textValue());
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToInt()) {
+ return defaultValue.intValue();
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
+ }
case TIME:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- return DateTimeUtil.isoTimeToMicros(defaultValue.textValue());
+ if (defaultValue.isTextual()) {
+ return DateTimeUtil.isoTimeToMicros(defaultValue.textValue());
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
+ }
case TIMESTAMP:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- if (((Types.TimestampType) type).shouldAdjustToUTC()) {
- String timestampTz = defaultValue.textValue();
- Preconditions.checkArgument(
- DateTimeUtil.isUTCTimestamptz(timestampTz),
- "Cannot parse default as a %s value: %s, offset must be +00:00",
- type,
- defaultValue);
- return DateTimeUtil.isoTimestamptzToMicros(timestampTz);
+ if (defaultValue.isTextual()) {
+ if (((Types.TimestampType) type).shouldAdjustToUTC()) {
+ String timestampTz = defaultValue.textValue();
+ Preconditions.checkArgument(
+ DateTimeUtil.isUTCTimestamptz(timestampTz),
+ "Cannot parse default as a %s value: %s, offset must be +00:00",
+ type,
+ defaultValue);
+ return DateTimeUtil.isoTimestamptzToMicros(timestampTz);
+ } else {
+ return DateTimeUtil.isoTimestampToMicros(defaultValue.textValue());
+ }
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
} else {
- return DateTimeUtil.isoTimestampToMicros(defaultValue.textValue());
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
}
case TIMESTAMP_NANO:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- if (((Types.TimestampNanoType) type).shouldAdjustToUTC()) {
- String timestampTzNano = defaultValue.textValue();
- Preconditions.checkArgument(
- DateTimeUtil.isUTCTimestamptz(timestampTzNano),
- "Cannot parse default as a %s value: %s, offset must be +00:00",
- type,
- defaultValue);
- return DateTimeUtil.isoTimestamptzToNanos(timestampTzNano);
+ if (defaultValue.isTextual()) {
+ if (((Types.TimestampNanoType) type).shouldAdjustToUTC()) {
+ String timestampTzNano = defaultValue.textValue();
+ Preconditions.checkArgument(
+ DateTimeUtil.isUTCTimestamptz(timestampTzNano),
+ "Cannot parse default as a %s value: %s, offset must be +00:00",
+ type,
+ defaultValue);
+ return DateTimeUtil.isoTimestamptzToNanos(timestampTzNano);
+ } else {
+ return DateTimeUtil.isoTimestampToNanos(defaultValue.textValue());
+ }
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
Review Comment:
now : Integer → SingleValueParser tries to parse as DATE → Expects string → Fails
##########
core/src/main/java/org/apache/iceberg/SingleValueParser.java:
##########
@@ -119,40 +119,60 @@ public static Object fromJson(Type type, JsonNode defaultValue) {
}
return uuid;
case DATE:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- return DateTimeUtil.isoDateToDays(defaultValue.textValue());
+ if (defaultValue.isTextual()) {
+ return DateTimeUtil.isoDateToDays(defaultValue.textValue());
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToInt()) {
+ return defaultValue.intValue();
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
+ }
case TIME:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- return DateTimeUtil.isoTimeToMicros(defaultValue.textValue());
+ if (defaultValue.isTextual()) {
+ return DateTimeUtil.isoTimeToMicros(defaultValue.textValue());
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
+ }
case TIMESTAMP:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- if (((Types.TimestampType) type).shouldAdjustToUTC()) {
- String timestampTz = defaultValue.textValue();
- Preconditions.checkArgument(
- DateTimeUtil.isUTCTimestamptz(timestampTz),
- "Cannot parse default as a %s value: %s, offset must be +00:00",
- type,
- defaultValue);
- return DateTimeUtil.isoTimestamptzToMicros(timestampTz);
+ if (defaultValue.isTextual()) {
+ if (((Types.TimestampType) type).shouldAdjustToUTC()) {
+ String timestampTz = defaultValue.textValue();
+ Preconditions.checkArgument(
+ DateTimeUtil.isUTCTimestamptz(timestampTz),
+ "Cannot parse default as a %s value: %s, offset must be +00:00",
+ type,
+ defaultValue);
+ return DateTimeUtil.isoTimestamptzToMicros(timestampTz);
+ } else {
+ return DateTimeUtil.isoTimestampToMicros(defaultValue.textValue());
+ }
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
} else {
- return DateTimeUtil.isoTimestampToMicros(defaultValue.textValue());
+ throw new IllegalArgumentException(
+ String.format("Cannot parse default as a %s value: %s", type, defaultValue));
}
case TIMESTAMP_NANO:
- Preconditions.checkArgument(
- defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
- if (((Types.TimestampNanoType) type).shouldAdjustToUTC()) {
- String timestampTzNano = defaultValue.textValue();
- Preconditions.checkArgument(
- DateTimeUtil.isUTCTimestamptz(timestampTzNano),
- "Cannot parse default as a %s value: %s, offset must be +00:00",
- type,
- defaultValue);
- return DateTimeUtil.isoTimestamptzToNanos(timestampTzNano);
+ if (defaultValue.isTextual()) {
+ if (((Types.TimestampNanoType) type).shouldAdjustToUTC()) {
+ String timestampTzNano = defaultValue.textValue();
+ Preconditions.checkArgument(
+ DateTimeUtil.isUTCTimestamptz(timestampTzNano),
+ "Cannot parse default as a %s value: %s, offset must be +00:00",
+ type,
+ defaultValue);
+ return DateTimeUtil.isoTimestamptzToNanos(timestampTzNano);
+ } else {
+ return DateTimeUtil.isoTimestampToNanos(defaultValue.textValue());
+ }
+ } else if (defaultValue.isIntegralNumber() && defaultValue.canConvertToLong()) {
+ return defaultValue.longValue();
Review Comment:
now : Integer → SingleValueParser tries to parse as DATE → Expects string
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org