You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2023/03/10 19:15:19 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request, #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
aokolnychyi opened a new pull request, #7068:
URL: https://github.com/apache/iceberg/pull/7068
This PR migrates `AddFilesProcedure` to use `ProcedureInput`.
--
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
[GitHub] [iceberg] szehon-ho commented on a diff in pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#discussion_r1132971162
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/ProcedureInput.java:
##########
@@ -167,6 +167,11 @@ public Identifier ident(ProcedureParameter param) {
return catalogAndIdent.identifier();
}
+ public Identifier ident(ProcedureParameter param, CatalogPlugin defaultCatalog) {
Review Comment:
I am probably a bit late to comment, but I wanted to note, I think 'defaultCatalog' is a bit confusing here, as ProcedureInput already comes with 'catalog'. To me, its more of a catalog override, isnt it?
--
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
[GitHub] [iceberg] aokolnychyi commented on pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#issuecomment-1464555102
Agreed, will cherry-pick in a bit.
--
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
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#discussion_r1132901856
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/ProcedureInput.java:
##########
@@ -167,6 +167,11 @@ public Identifier ident(ProcedureParameter param) {
return catalogAndIdent.identifier();
}
+ public Identifier ident(ProcedureParameter param, CatalogPlugin defaultCatalog) {
Review Comment:
I think we can't use it as we need to validate the catalog in that method. However, I changed the code a bit to share more. @singhpk234, could you take another 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
[GitHub] [iceberg] flyrain commented on pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#issuecomment-1464552650
Yeah, my only concern is that, any new change to procedure will be hard to apply to 3.2 if we don't do that.
--
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
[GitHub] [iceberg] aokolnychyi commented on pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#issuecomment-1464544455
> Do we need the ProcedureInput change in Spark 3.2?
@flyrain, I was debating that. Probably we could do that for consistency? What are your thoughts?
--
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
[GitHub] [iceberg] aokolnychyi merged pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #7068:
URL: https://github.com/apache/iceberg/pull/7068
--
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
[GitHub] [iceberg] aokolnychyi commented on pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#issuecomment-1464562239
Thanks for reviewing, @singhpk234 and @flyrain!
--
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
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#discussion_r1133029178
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/ProcedureInput.java:
##########
@@ -167,6 +167,11 @@ public Identifier ident(ProcedureParameter param) {
return catalogAndIdent.identifier();
}
+ public Identifier ident(ProcedureParameter param, CatalogPlugin defaultCatalog) {
Review Comment:
Well, it kind of overrides the default catalog to use if the identifier does not reference a catalog explicitly. It is not necessarily the catalog that will be used. It matches the arg name in `Spark3Util`.
--
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
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#discussion_r1132919112
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/ProcedureInput.java:
##########
@@ -167,6 +167,11 @@ public Identifier ident(ProcedureParameter param) {
return catalogAndIdent.identifier();
}
+ public Identifier ident(ProcedureParameter param, CatalogPlugin defaultCatalog) {
Review Comment:
Agree, by above, I meant that modifying this function to just get identifier from `ident(ProcedureParameter param, CatalogPlugin defaultCatalog)` and keep the assertion as it is, hence specifically line 158.
---
> However, I changed the code a bit to share more. @singhpk234, could you take another look?
+1 This looks great !
--
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
[GitHub] [iceberg] singhpk234 commented on a diff in pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#discussion_r1132879023
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/procedures/ProcedureInput.java:
##########
@@ -167,6 +167,11 @@ public Identifier ident(ProcedureParameter param) {
return catalogAndIdent.identifier();
}
+ public Identifier ident(ProcedureParameter param, CatalogPlugin defaultCatalog) {
Review Comment:
[minor] can we use this function in L158, to re-use it ?
--
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
[GitHub] [iceberg] aokolnychyi commented on pull request #7068: Spark 3.3: Use ProcedureInput in AddFilesProcedure
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #7068:
URL: https://github.com/apache/iceberg/pull/7068#issuecomment-1464279981
cc @szehon-ho @amogh-jahagirdar @flyrain @singhpk234 @ajantha-bhat
--
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