[SPARK-42901][CONNECT][PYTHON] Move StorageLevel into a separate file to avoid potential file recursively imports
#40518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LuciferYang
changed the title
StorageLevel into a separate file to avoid potential file recursively importsStorageLevel into a separate file to avoid potential file recursively imports
LuciferYang
commented
Mar 22, 2023
| option java_package = "org.apache.spark.connect.proto"; | ||
|
|
||
| // StorageLevel for persisting Datasets/Tables. | ||
| message StorageLevel { |
There was a problem hiding this comment.
cc @HyukjinKwon
HyukjinKwon
reviewed
Mar 22, 2023
| @@ -0,0 +1,37 @@ | |||
| /* | |||
There was a problem hiding this comment.
Should we rename this to something else at a high level? e.g., common.proto or utils.proto. Otherwise, LGTM from my end.
There was a problem hiding this comment.
Yes, I also think there should be a high level name, let me change it to common.proto first. If others have better suggestions, I will rename it again
There was a problem hiding this comment.
I found that connect-gen-protos.sh won't automatically delete storage_level_pb2.py & storage_level_pb2.pyi, hahaha
ueshin
approved these changes
Mar 22, 2023
|
rebase due to #40516 merged |
There was a problem hiding this comment.
@LuciferYang . This looks worthy of having a new JIRA. Please create a new JIRA for this PR and use it. This PR is a good contribution of yours. 😄
|
@LuciferYang nit: you need to update the PR description. There is an old file name |
LuciferYang
changed the title
StorageLevel into a separate file to avoid potential file recursively importsStorageLevel into a separate file to avoid potential file recursively imports
LuciferYang
changed the title
StorageLevel into a separate file to avoid potential file recursively importsStorageLevel into a separate file to avoid potential file recursively imports
Thanks ~ fixed |
@dongjoon-hyun Thanks for your suggestion ~ I created SPARK-42901 and updated the pr title 😄 |
|
GA passed ~ |
HyukjinKwon
approved these changes
Mar 23, 2023
|
Merged to master and branch-3.4. |
HyukjinKwon
pushed a commit
that referenced
this pull request
Mar 23, 2023
…le to avoid potential `file recursively imports` #40510 introduce `message StorageLevel` to `base.proto`, but if we try to import `base.proto` in `catalog.proto` to reuse `StorageLevel` in `message CacheTable` and run `build/sbt "connect-common/compile" to compile, there will be following message in compile log: ``` spark/connect/base.proto:23:1: File recursively imports itself: spark/connect/base.proto -> spark/connect/commands.proto -> spark/connect/relations.proto -> spark/connect/catalog.proto -> spark/connect/base.proto spark/connect/catalog.proto:22:1: Import "spark/connect/base.proto" was not found or had errors. spark/connect/catalog.proto:144:12: "spark.connect.DataType" seems to be defined in "spark/connect/types.proto", which is not imported by "spark/connect/catalog.proto". To use it here, please add the necessary import. spark/connect/catalog.proto:161:12: "spark.connect.DataType" seems to be defined in "spark/connect/types.proto", which is not imported by "spark/connect/catalog.proto". To use it here, please add the necessary import. spark/connect/relations.proto:25:1: Import "spark/connect/catalog.proto" was not found or had errors. spark/connect/relations.proto:84:5: "Catalog" is not defined. spark/connect/commands.proto:22:1: Import "spark/connect/relations.proto" was not found or had errors. spark/connect/commands.proto:63:3: "Relation" is not defined. spark/connect/commands.proto:81:3: "Relation" is not defined. spark/connect/commands.proto:142:3: "Relation" is not defined. spark/connect/base.proto:23:1: Import "spark/connect/commands.proto" was not found or had errors. spark/connect/base.proto:25:1: Import "spark/connect/relations.proto" was not found or had errors. .... ``` So this pr move `message StorageLevel` to a separate file to avoid this potential file recursively imports. To avoid potential file recursively imports. No - Pass GitHub Actions - Manual check: - Add `import "spark/connect/common.proto";` to `catalog.proto` - run `build/sbt "connect-common/compile"` No compilation logs related to `File recursively imports itself` . Closes #40518 from LuciferYang/SPARK-42889-FOLLOWUP. Lead-authored-by: yangjie01 <yangjie01@baidu.com> Co-authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 88cc239) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
|
Thanks @HyukjinKwon @dongjoon-hyun @ueshin |
|
+1, late LGTM. |
snmvaughan
pushed a commit
to snmvaughan/spark
that referenced
this pull request
Jun 20, 2023
…le to avoid potential `file recursively imports` apache#40510 introduce `message StorageLevel` to `base.proto`, but if we try to import `base.proto` in `catalog.proto` to reuse `StorageLevel` in `message CacheTable` and run `build/sbt "connect-common/compile" to compile, there will be following message in compile log: ``` spark/connect/base.proto:23:1: File recursively imports itself: spark/connect/base.proto -> spark/connect/commands.proto -> spark/connect/relations.proto -> spark/connect/catalog.proto -> spark/connect/base.proto spark/connect/catalog.proto:22:1: Import "spark/connect/base.proto" was not found or had errors. spark/connect/catalog.proto:144:12: "spark.connect.DataType" seems to be defined in "spark/connect/types.proto", which is not imported by "spark/connect/catalog.proto". To use it here, please add the necessary import. spark/connect/catalog.proto:161:12: "spark.connect.DataType" seems to be defined in "spark/connect/types.proto", which is not imported by "spark/connect/catalog.proto". To use it here, please add the necessary import. spark/connect/relations.proto:25:1: Import "spark/connect/catalog.proto" was not found or had errors. spark/connect/relations.proto:84:5: "Catalog" is not defined. spark/connect/commands.proto:22:1: Import "spark/connect/relations.proto" was not found or had errors. spark/connect/commands.proto:63:3: "Relation" is not defined. spark/connect/commands.proto:81:3: "Relation" is not defined. spark/connect/commands.proto:142:3: "Relation" is not defined. spark/connect/base.proto:23:1: Import "spark/connect/commands.proto" was not found or had errors. spark/connect/base.proto:25:1: Import "spark/connect/relations.proto" was not found or had errors. .... ``` So this pr move `message StorageLevel` to a separate file to avoid this potential file recursively imports. To avoid potential file recursively imports. No - Pass GitHub Actions - Manual check: - Add `import "spark/connect/common.proto";` to `catalog.proto` - run `build/sbt "connect-common/compile"` No compilation logs related to `File recursively imports itself` . Closes apache#40518 from LuciferYang/SPARK-42889-FOLLOWUP. Lead-authored-by: yangjie01 <yangjie01@baidu.com> Co-authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 88cc239) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
#40510 introduce
message StorageLeveltobase.proto, but if we try to importbase.protoincatalog.prototo reuseStorageLevelinmessage CacheTableand run `build/sbt "connect-common/compile" to compile, there will be following message in compile log:So this pr move
message StorageLevelto a separate file to avoid this potential file recursively imports.Why are the changes needed?
To avoid potential file recursively imports.
Does this PR introduce any user-facing change?
No
How was this patch tested?
import "spark/connect/common.proto";tocatalog.protobuild/sbt "connect-common/compile"No compilation logs related to
File recursively imports itself.