You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mattheusv (via GitHub)" <gi...@apache.org> on 2024/09/06 18:52:28 UTC
[PR] scan: fix error when reading an empty table [iceberg-rust]
mattheusv opened a new pull request, #608:
URL: https://github.com/apache/iceberg-rust/pull/608
Previously TableScan struct was requiring a Snapshot to plan files and for empty tables without a snapshot an error was being returned instead of an empty result.
Following the same approach of Java [0] and Python [1] implementation this commit change the snapshot property to accept None values and the `plan_files` method was also changed to return an empty stream if the snapshot is not present on on PlanContext.
[0] https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/SnapshotScan.java#L119
[1] https://github.com/apache/iceberg-python/blob/main/pyiceberg/table/__init__.py#L1979
Fixes: https://github.com/apache/iceberg-rust/issues/580
--
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] scan: fix error when reading an empty table [iceberg-rust]
Posted by "mattheusv (via GitHub)" <gi...@apache.org>.
mattheusv commented on PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608#issuecomment-2341807794
@sdd I've changed the code to return a `ErrorKind::TableWithoutSnapshot` instead of FeatureUnsupported. With this the user can differentiate\ a table without snapshots and a table without data. WYT?
--
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] scan: fix error when reading an empty table [iceberg-rust]
Posted by "mattheusv (via GitHub)" <gi...@apache.org>.
mattheusv commented on PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608#issuecomment-2338025932
Hi @sdd , thanks for your review.
I'm not sure if I understand your suggestion. I agree that would be better to fix this edge case with a smaller change, but I'm not sure If I understand your suggestion correctly.
The idea would be make the callers of `TableScanBuilder.build()` to handle the case where the table don't have any data? The `scan_builder.build()` currently returns a `TableScan` and the `TableScan.plan_files` that actually may return a `stream::empty().boxed()`, so I don't know if I'm missing something here? (I'm new on this codebase)
Just adding another idea: would make sense to return an error like `Error::new(ErrorKind::EmptyTable)` when calling `TableScanBuilder.build()`?
--
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] scan: fix error when reading an empty table [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608#issuecomment-2338064061
Just to clarify, not having any snapshots is not necessarily the same as not having any data. If there is no current snapshot then there can't be any data, but someone could delete all data from a table, resulting in there being a snapshot, but no data. The existing code would handle this second case just fine - we only need to handle the issue of no snapshots.
--
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] scan: change ErrorKind when table dont have spanshots [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608
--
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] scan: change ErrorKind when table dont have spanshots [iceberg-rust]
Posted by "mattheusv (via GitHub)" <gi...@apache.org>.
mattheusv commented on PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608#issuecomment-2373808379
> > We've been very selective when it comes to adding new values to `ErrorKind`. I'd personally go for `Unexpected` here - but maybe @liurenjie1024 or @Xuanwo can confirm what would be best.
>
> Yes, I'm on `Unexpected` too, except this error kind is meaningful for users to make decisions.
@Xuanwo @sdd could you guys please take a look?
--
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] scan: fix error when reading an empty table [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608#issuecomment-2344385358
We've been very selective when it comes to adding new values to `ErrorKind`. I'd personally go for `Unexpected` here - but maybe @liurenjie1024 or @Xuanwo can confirm what would be best.
--
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] scan: fix error when reading an empty table [iceberg-rust]
Posted by "mattheusv (via GitHub)" <gi...@apache.org>.
mattheusv commented on PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608#issuecomment-2334642109
Just some notes for reviewers:
I'm not 100% sure that this the best approach to fix this issue, I've just tried to follow the same approach used on Java and Python implementation, but I don't know if there is a better way to implement in Rust.
Another point is that I'm bit confusing where should I write a test case for this issue?
--
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] scan: fix error when reading an empty table [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608#issuecomment-2334779317
Thanks for the contribution! Do we need to address this inside scan though? Why let someone build a `TableScan` that will always be useless?
This can be handled instead in the code that invokes `table.scan()`, without needing to make changes to the scan builder, scan, and context objects just for this edge case.
```rust
let scan_builder = table.scan();
// (customize builder here if reqd)...
let Ok(scan) = table_scan_builder.build() else {
return Ok(stream::empty().boxed());
};
scan().plan_files()
```
--
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] scan: fix error when reading an empty table [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #608:
URL: https://github.com/apache/iceberg-rust/pull/608#issuecomment-2368247337
> We've been very selective when it comes to adding new values to `ErrorKind`. I'd personally go for `Unexpected` here - but maybe @liurenjie1024 or @Xuanwo can confirm what would be best.
Yes, I'm on `Unexpected` too, except this error kind is meaningful for users to make decisions.
--
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