PHOENIX-6783 Add spark-sql support for spark2 datasourceV2 and spark3#139
Merged
stoty merged 1 commit intoapache:masterfrom Sep 18, 2024
Merged
PHOENIX-6783 Add spark-sql support for spark2 datasourceV2 and spark3#139stoty merged 1 commit intoapache:masterfrom
stoty merged 1 commit intoapache:masterfrom
Conversation
Contributor
rejeb
force-pushed
the
phoenix5_spark_sql_support
branch
from
e9dab6a to
0c19440
Compare
Contributor
|
💔 -1 overall
This message was automatically generated. |
rejeb
changed the title
rejeb
changed the title
Contributor
|
Please keep the DataSource V1 code. |
rejeb
force-pushed
the
phoenix5_spark_sql_support
branch
from
0c19440 to
8750c46
Compare
Contributor
Author
|
Rollback datasourceV1 and add tests. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
@stoty, can you review please. |
stoty
suggested changes
Aug 28, 2024
Contributor
stoty
left a comment
stoty
left a comment
There was a problem hiding this comment.
Basically looks good, but I added a lot of nits/cleanup suggestions.
phoenix5-spark/README.md
Outdated
| .read() | ||
| .format("phoenix") | ||
| .option("table", "TABLE1") | ||
| .option("jdbcUrl", "jdbc:phoenix:zkHost:zkport") |
Contributor
There was a problem hiding this comment.
see above, this applies to all uses with explicit jdbcUrl
phoenix5-spark/src/main/scala/org/apache/phoenix/spark/PhoenixRDD.scala
Outdated
Show resolved
Hide resolved
phoenix5-spark/src/main/scala/org/apache/phoenix/spark/PhoenixRelation.scala
Outdated
Show resolved
Hide resolved
...5-spark/src/main/scala/org/apache/phoenix/spark/datasource/v2/FilterExpressionCompiler.scala
Outdated
Show resolved
Hide resolved
...x5-spark/src/main/scala/org/apache/phoenix/spark/datasource/v2/PhoenixSparkSqlRelation.scala
Show resolved
Hide resolved
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
@stoty, than you for the review. I made the changes as you suggest and added comments. |
Contributor
That's not what I can see. Please check again, and if you still find that they are pure Phoenix tests, point out why the code to perform the queries via Spark does not work. (executeQuery() override ) |
Contributor
Author
|
I haven't paid attention to the executeQuery method. You're rigth, the tests are executed throw spark. I rolled back my modifications and done the same for OrderByIT as I made a change in the same way fot it. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
stoty
suggested changes
Sep 10, 2024
Contributor
stoty
left a comment
stoty
left a comment
There was a problem hiding this comment.
Generally l looks good, I added some nits and minor requests.
phoenix5-spark/README.md
Outdated
| .save(); | ||
|
|
||
| jsc.stop(); | ||
Contributor
There was a problem hiding this comment.
Nit:
I can't see in this editor. Is this extra whitespace ? If yes, then please delete
...ark/src/main/java/org/apache/phoenix/spark/datasource/v2/reader/PhoenixDataSourceReader.java
Show resolved
Hide resolved
...x5-spark/src/main/scala/org/apache/phoenix/spark/datasource/v2/PhoenixSparkSqlRelation.scala
Show resolved
Hide resolved
rejeb
force-pushed
the
phoenix5_spark_sql_support
branch
from
aef9db2 to
631b0ec
Compare
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
I have re-started the test to see if the first failure was a transient. |
Contributor
|
It's a major pain to identify any problem from the 40MB scalatest output. |
Contributor
|
My local runs also fail:
|
Contributor
|
Looks like you need to reset PhoenixTestingDataSourceWriter before using it, as now you are using it from two tests. |
Contributor
Author
|
Hi @stoty, |
Contributor
Author
Unzipped file is about 450MB and most of the logs are debug logs essentially hbase logs. IDK, if it is helfull to have these logs during tests. |
rejeb
force-pushed
the
phoenix5_spark_sql_support
branch
from
cc645f8 to
6750b0c
Compare
rejeb
force-pushed
the
phoenix5_spark_sql_support
branch
from
6750b0c to
eda8315
Compare
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
We've tried that, but then we couldn't debug some test failures that only happened in the CI environment. |
Contributor
|
Please also rename the JIRA to match the commit message (which is more descriptive). |
Contributor
Author
Done. |
Contributor
|
Updated the commit message to match the PR description on commit. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
ZOOKEEPER_URLpropety for all tests withJDBC_URL.To discuss: Datasouce v1 is deprecated and have no tests. I removed datasource V1 classes. Is it OK or should I revert the change and add tests ?