You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "PorterZhang2021 (via GitHub)" <gi...@apache.org> on 2024/04/21 22:37:50 UTC
[PR] [Kyuubi 6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
PorterZhang2021 opened a new pull request, #6330:
URL: https://github.com/apache/kyuubi/pull/6330
# :mag: Description
## Issue References π
<!-- Append the issue number after #. If there is no issue for you to link create one or -->
<!-- If there are no issues to link, please provide details here. -->
This pull request fixes #6305
## Describe Your Solution π§
### Solution 1 use `<profile>` - Inappropriate
I found a way to use <profiles>, roughly as follows:
```java
<profiles>
<profile>
<id>scala-2.12</id>
<activation>
<activeByDefault>true</activeByDefault>
</activation>
<properties>
<scala.binary.version>2.12</scala.binary.version>
</properties>
</profile>
<profile>
<id>scala-2.13</id>
<properties>
<scala.binary.version>2.13</scala.binary.version>
</properties>
</profile>
</profiles>
```
After specifying, I attempted to use
1. `/ Build/mvn install - DskipTests - Dmaven. savadoc. skip=true - Dmaven. scaladoc. skip=true - Dmaven. source. skip - Pscala-2.12- Pscala-2.13- pl: kyuubi spark SQL engine - am`
2. `/ Build/mvn install - DskipTests - Dmaven. savadoc. skip=true - Dmaven. scaladoc. skip=true - Dmaven. source. skip - Pscala-2.13- Pscala-2.12- pl: kyuubi spark SQL engine - am`
#### Problem
But in the end, it was found that if both '- Pscala2.12' and '- Pscala2.13' are used at the same time, '- Pscala2.13' will be selected by default, which may not be a good solution to this problem.
### Solution2
Later, I thought about whether it was possible to filter the parameter '$@' internally.It is effective.
## Types of changes :bookmark:
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
## Test Plan π§ͺ
1. scala2.12
2. scala2.13
---
# Checklist π
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)
**Be nice. Be informative.**
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#issuecomment-2111571947
Thanks, merged to master
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #6330: [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13
URL: https://github.com/apache/kyuubi/pull/6330
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#issuecomment-2068642026
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/6330?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 58.37%. Comparing base [(`90491fc`)](https://app.codecov.io/gh/apache/kyuubi/commit/90491fc07eb895c9b773cce94dbe54ba636c11f5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`3d2926a`)](https://app.codecov.io/gh/apache/kyuubi/pull/6330?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 2 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #6330 +/- ##
============================================
- Coverage 58.45% 58.37% -0.08%
Complexity 24 24
============================================
Files 653 653
Lines 39834 39834
Branches 5477 5477
============================================
- Hits 23283 23255 -28
- Misses 14064 14085 +21
- Partials 2487 2494 +7
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/kyuubi/pull/6330?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#issuecomment-2081855554
OCR means "Optical Character Recognition", what I mean strange part is
<img width="833" alt="image" src="https://github.com/apache/kyuubi/assets/26535726/cc3bc40a-14b4-46f2-97d0-bbbd06d489e7">
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "yikf (via GitHub)" <gi...@apache.org>.
yikf commented on code in PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#discussion_r1574221341
##########
build/dist:
##########
@@ -247,18 +247,25 @@ BUILD_COMMAND=("$MVN" clean install $MVN_DIST_OPT $@)
echo -e "\nBuilding with..."
# shellcheck disable=SC2145
echo -e "\$ ${BUILD_COMMAND[@]}\n"
+"${BUILD_COMMAND[@]}"
+FILTERED_ARGS=()
+# shellcheck disable=SC2045
+for arg in "$@"; do
Review Comment:
The logic can be a function so that other modules/logics do not need to rely on the context of the script.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "PorterZhang2021 (via GitHub)" <gi...@apache.org>.
PorterZhang2021 commented on PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#issuecomment-2081217136
Hello , I have revised the filtering according to @yikf 's suggestion, but it is not easy to solve the problem of `- p scala-2.13 p1`. Therefore, I have written a simple prompt for users to use the `- pscala-2.13,p1` method, cc @yikf @pan3793
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "yikf (via GitHub)" <gi...@apache.org>.
yikf commented on code in PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#discussion_r1574190693
##########
build/dist:
##########
@@ -247,18 +247,25 @@ BUILD_COMMAND=("$MVN" clean install $MVN_DIST_OPT $@)
echo -e "\nBuilding with..."
# shellcheck disable=SC2145
echo -e "\$ ${BUILD_COMMAND[@]}\n"
+"${BUILD_COMMAND[@]}"
+FILTERED_ARGS=()
+# shellcheck disable=SC2045
+for arg in "$@"; do
+ if [[ $arg != -Pscala* ]]; then
+ FILTERED_ARGS+=("$arg")
+ fi
+done
# shellcheck disable=SC2050
if [ "$SCALA_VERSION" = "2.12" ]; then
- EXTRA_SPARK_ENGINE_BUILD_COMMAND=("$MVN" install $MVN_DIST_OPT $@ -Pscala-2.13 -pl :kyuubi-spark-sql-engine_2.13 -am)
+ EXTRA_SPARK_ENGINE_BUILD_COMMAND=("$MVN" install $MVN_DIST_OPT -Pscala-2.13 ${FILTERED_ARGS[@]} -pl :kyuubi-spark-sql-engine_2.13 -am)
else
- EXTRA_SPARK_ENGINE_BUILD_COMMAND=("$MVN" install $MVN_DIST_OPT $@ -Pscala-2.12 -pl :kyuubi-spark-sql-engine_2.12 -am)
+ EXTRA_SPARK_ENGINE_BUILD_COMMAND=("$MVN" install $MVN_DIST_OPT -pl ${FILTERED_ARGS[@]} :kyuubi-spark-sql-engine_2.12 -am)
Review Comment:
```suggestion
EXTRA_SPARK_ENGINE_BUILD_COMMAND=("$MVN" install $MVN_DIST_OPT ${FILTERED_ARGS[@]} -pl :kyuubi-spark-sql-engine_2.12 -am)
```
##########
build/dist:
##########
@@ -247,18 +247,25 @@ BUILD_COMMAND=("$MVN" clean install $MVN_DIST_OPT $@)
echo -e "\nBuilding with..."
# shellcheck disable=SC2145
echo -e "\$ ${BUILD_COMMAND[@]}\n"
+"${BUILD_COMMAND[@]}"
+FILTERED_ARGS=()
+# shellcheck disable=SC2045
+for arg in "$@"; do
+ if [[ $arg != -Pscala* ]]; then
Review Comment:
I agree with the approach of dealing with parameters If there's no better way, even if it is hack to compare.
Not only are we dealing with arguments like -Pscala*, but we also need -P p1,scala-2.13, -P p1,scala-2.13,p2... -Dscala.version=2.13 and so on.
In addition, we can add some comments to describe it, and if the scala version changes in the future, it can guide the developer to clarify the meaning of this piece and determine whether it needs to change.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#issuecomment-2081464973
BTW, the code you pasted is weird, like something produced by OCR?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#issuecomment-2081463750
@PorterZhang2021 thanks for keeping work on this issue. actually, we can replace all mvn args `scala-2.12` with `scala-2.13` and vice versa
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "PorterZhang2021 (via GitHub)" <gi...@apache.org>.
PorterZhang2021 commented on PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#issuecomment-2081813627
> @PorterZhang2021 thanks for keeping work on this issue. actually, we can replace all mvn args `scala-2.12` with `scala-2.13` and vice versa
ok, I will try it again, I'm not quite sure what ocr means. Perhaps there is a part of the content that I searched for Google and inquired about GPT, and then made similar implementations myself, which looks a bit strange. I will make some modifications later.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #6305][FOLLOWUP] Improve package Spark SQL engine both Scala 2.12 and 2.13 [kyuubi]
Posted by "PorterZhang2021 (via GitHub)" <gi...@apache.org>.
PorterZhang2021 commented on PR #6330:
URL: https://github.com/apache/kyuubi/pull/6330#issuecomment-2097193094
cc @pan3793
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org