You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2023/03/22 08:25:42 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #40518: [SPARK-42889][CONNECT][FOLLOWUP] Move `StorageLevel` into a separate file to avoid potential file recursively imports
LuciferYang opened a new pull request, #40518:
URL: https://github.com/apache/spark/pull/40518
### What changes were proposed in this pull request?
https://github.com/apache/spark/pull/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.
### 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?
Pass GitHub Actions
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #40518: [SPARK-42889][CONNECT][PYTHON][FOLLOWUP] Move `StorageLevel` into a separate file to avoid potential file recursively imports
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40518:
URL: https://github.com/apache/spark/pull/40518#discussion_r1144391398
##########
connector/connect/common/src/main/protobuf/spark/connect/storage_level.proto:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+syntax = 'proto3';
+
+package spark.connect;
+
+option java_multiple_files = true;
+option java_package = "org.apache.spark.connect.proto";
+
+// StorageLevel for persisting Datasets/Tables.
+message StorageLevel {
Review Comment:
cc @HyukjinKwon
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ueshin commented on pull request #40518: [SPARK-42889][CONNECT][PYTHON][FOLLOWUP] Move `StorageLevel` into a separate file to avoid potential file recursively imports
Posted by "ueshin (via GitHub)" <gi...@apache.org>.
ueshin commented on PR #40518:
URL: https://github.com/apache/spark/pull/40518#issuecomment-1480408180
@LuciferYang nit: you need to update the PR description. There is an old file name `storage_level.proto`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40518: [SPARK-42889][CONNECT][PYTHON][FOLLOWUP] Move `StorageLevel` into a separate file to avoid potential file recursively imports
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40518:
URL: https://github.com/apache/spark/pull/40518#discussion_r1144792924
##########
connector/connect/common/src/main/protobuf/spark/connect/storage_level.proto:
##########
@@ -0,0 +1,37 @@
+/*
Review Comment:
Should we rename this to something else at a high level? e.g., common.proto or utils.proto. Otherwise, LGTM from my end.
cc @hvanhovell @grundprinzip @ueshin
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #40518: [SPARK-42901][CONNECT][PYTHON] Move `StorageLevel` into a separate file to avoid potential `file recursively imports`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #40518:
URL: https://github.com/apache/spark/pull/40518#issuecomment-1480558008
Merged to master and branch-3.4.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #40518: [SPARK-42889][CONNECT][PYTHON][FOLLOWUP] Move `StorageLevel` into a separate file to avoid potential file recursively imports
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #40518:
URL: https://github.com/apache/spark/pull/40518#issuecomment-1480402574
rebase due to https://github.com/apache/spark/pull/40516 merged
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #40518: [SPARK-42901][CONNECT][PYTHON] Move `StorageLevel` into a separate file to avoid potential `file recursively imports`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #40518:
URL: https://github.com/apache/spark/pull/40518#issuecomment-1480591952
Thanks @HyukjinKwon @dongjoon-hyun @ueshin
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #40518: [SPARK-42901][CONNECT][PYTHON] Move `StorageLevel` into a separate file to avoid potential `file recursively imports`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #40518: [SPARK-42901][CONNECT][PYTHON] Move `StorageLevel` into a separate file to avoid potential `file recursively imports`
URL: https://github.com/apache/spark/pull/40518
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #40518: [SPARK-42889][CONNECT][PYTHON][FOLLOWUP] Move `StorageLevel` into a separate file to avoid potential file recursively imports
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40518:
URL: https://github.com/apache/spark/pull/40518#discussion_r1144813518
##########
connector/connect/common/src/main/protobuf/spark/connect/storage_level.proto:
##########
@@ -0,0 +1,37 @@
+/*
Review Comment:
I found that `connect-gen-protos.sh` won't automatically delete `storage_level_pb2.py` & `storage_level_pb2.pyi`, hahaha
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #40518: [SPARK-42901][CONNECT][PYTHON] Move `StorageLevel` into a separate file to avoid potential `file recursively imports`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #40518:
URL: https://github.com/apache/spark/pull/40518#issuecomment-1480556858
GA passed ~
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #40518: [SPARK-42889][CONNECT][PYTHON][FOLLOWUP] Move `StorageLevel` into a separate file to avoid potential file recursively imports
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #40518:
URL: https://github.com/apache/spark/pull/40518#discussion_r1144799891
##########
connector/connect/common/src/main/protobuf/spark/connect/storage_level.proto:
##########
@@ -0,0 +1,37 @@
+/*
Review 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
##########
connector/connect/common/src/main/protobuf/spark/connect/storage_level.proto:
##########
@@ -0,0 +1,37 @@
+/*
Review 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
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #40518: [SPARK-42901][CONNECT][PYTHON] Move `StorageLevel` into a separate file to avoid potential `file recursively imports`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #40518:
URL: https://github.com/apache/spark/pull/40518#issuecomment-1480438169
> @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. 😄
@dongjoon-hyun Thanks for your suggestion ~ I created SPARK-42901 and updated the pr title 😄
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ueshin commented on a diff in pull request #40518: [SPARK-42889][CONNECT][PYTHON][FOLLOWUP] Move `StorageLevel` into a separate file to avoid potential file recursively imports
Posted by "ueshin (via GitHub)" <gi...@apache.org>.
ueshin commented on code in PR #40518:
URL: https://github.com/apache/spark/pull/40518#discussion_r1145188951
##########
connector/connect/common/src/main/protobuf/spark/connect/storage_level.proto:
##########
@@ -0,0 +1,37 @@
+/*
Review Comment:
`common.proto` sounds good to me.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40518: [SPARK-42901][CONNECT][PYTHON] Move `StorageLevel` into a separate file to avoid potential `file recursively imports`
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40518:
URL: https://github.com/apache/spark/pull/40518#issuecomment-1481795559
+1, late LGTM.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #40518: [SPARK-42901][CONNECT][PYTHON] Move `StorageLevel` into a separate file to avoid potential `file recursively imports`
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #40518:
URL: https://github.com/apache/spark/pull/40518#issuecomment-1480437248
> @LuciferYang nit: you need to update the PR description. There is an old file name `storage_level.proto`.
Thanks ~ fixed
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org