Spark3.1, Spark3.2, Spark3.3: Support setting current snapshot with ref #8392
Conversation
manuzhang
force-pushed
the
backport-8163
branch
3 times, most recently
from
3af9e21
to
be80598
Compare
|
@aokolnychyi @nastra please help review. |
...sions/src/test/java/org/apache/iceberg/spark/extensions/TestSetCurrentSnapshotProcedure.java
Outdated
Show resolved
Hide resolved
Back-port of apache#8163 to `spark/v3.3`, `spark/v3.2` and `spark/v3.1`
manuzhang
force-pushed
the
backport-8163
branch
from
be80598
to
7f04c3f
Compare
|
@nastra @amogh-jahagirdar please take another look |
There was a problem hiding this comment.
Thanks for the PR @manuzhang, I think we need to revisit the error messages being surfaced and the exception change for the case where there are missing parameters. See my comment below. If it's the case we should get a fix in for 3.4 as well.
| AnalysisException.class, | ||
| "Missing required parameters", | ||
| IllegalArgumentException.class, | ||
| "Either snapshot_id or ref must be provided, not both", |
There was a problem hiding this comment.
This error message doesn't seem right for the arguments that are being passed in. We're missing the required parameters right? Also I don't think we should change the exception that is being thrown for the existing cases (missing required parameters) because we probably want to preserve that behavior in case users rely on it. For the new failure case (ref + snapshot ID) the IllegalArgumentException is fine.
There was a problem hiding this comment.
Do you mean throw AnalysisException when both snapshot_id and ref are missing?
There was a problem hiding this comment.
@aokolnychyi what do you think?
There was a problem hiding this comment.
@amogh-jahagirdar, @aokolnychyi please share your thoughts here.
Back-port of #8163 to
spark/v3.3,spark/v3.2andspark/v3.1