PHOENIX-7407 Remove deprecated datasource V1 code from spark2 and spark3 connector#145
Merged
stoty merged 1 commit intoapache:masterfrom Jun 2, 2025
Merged
PHOENIX-7407 Remove deprecated datasource V1 code from spark2 and spark3 connector#145stoty merged 1 commit intoapache:masterfrom
stoty merged 1 commit intoapache:masterfrom
Conversation
Contributor
Contributor
|
Contributor
Author
|
I added tests for datasource v1 when I've been working on PHOENIX-6783 because there has been no tests before that. |
Contributor
Author
|
I added steps in github action workflow to generate test reports. The steps has failed with error "Error: HttpError: Resource not accessible by integration". |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
I have not forgotten about this, but I need to take care of some higher priority stuff first. |
Contributor
Author
I got some free time this weekend so I wanted to test something. Please do not mind about future pushes, I will post a message when I finish updates. I will create a new class with kept and added tests for V1 and helper methods then remove old class to make review easier. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
💔 -1 overall
This message was automatically generated. |
rejeb
changed the title
Contributor
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
Yetus is supposed to be fixed, kicking off a CI run. |
Contributor
|
Regarding the savemode changes: |
rejeb
force-pushed
the
PHOENIX-7407
branch
3 times, most recently
from
d1ea8ba to
fb0a8b1
Compare
Contributor
Author
@stoty, done. |
Contributor
|
Contributor
Author
It should truncate table and then insert data. |
Contributor
Thanks. |
Contributor
|
Also there are still a few unused imports and similar Yetus warnings. |
Contributor
Author
Yes, we can log something at INF., In this PR, I kept the actual behavior for Overwrite it means that it will not do the truncate before inserting data. |
Contributor
That's good. |
rejeb
force-pushed
the
PHOENIX-7407
branch
2 times, most recently
from
b271f60 to
a21759e
Compare
stoty
suggested changes
May 30, 2025
Contributor
stoty
left a comment
stoty
left a comment
There was a problem hiding this comment.
Please also fix the whitespace and codestyle issues reported by Yetus.
| JavaSparkContext jsc = new JavaSparkContext(sparkConf); | ||
| SQLContext sqlContext = new SQLContext(jsc); | ||
|
|
||
| SparkSession spark = SparkUtil.getSparkSession(); |
Contributor
There was a problem hiding this comment.
Looks like spark.hadoopRDD.ignoreEmptySplits is default since 3.2.0, so removing it should be OK.
Contributor
Author
There was a problem hiding this comment.
I just replaced old code with a call to existing SparkUtil.getSparkSession() to avoid having duplicate code.
Contributor
There was a problem hiding this comment.
That one doesn't have spark.hadoopRDD.ignoreEmptySplits for Spark3 (which is fine)
| JavaSparkContext jsc = new JavaSparkContext(sparkConf); | ||
| SQLContext sqlContext = new SQLContext(jsc); | ||
|
|
||
| SparkSession spark = SparkUtil.getSparkSession(); |
Contributor
There was a problem hiding this comment.
since 3.2.0 / SPARK-34809 spark.hadoopRDD.ignoreEmptySplits is enabled by default.
However, this uses Spark 2. Shouldn't we keep that property for Spark 2 ?
Contributor
Author
There was a problem hiding this comment.
I just replaced old code with a call to existing SparkUtil.getSparkSession() to avoid having duplicate code and the property is there.
| @@ -1,3 +1,21 @@ | |||
| /* | |||
Contributor
There was a problem hiding this comment.
Thanks.
As you are the orginal author, we can handle this here instead of a separate ticket.
phoenix5-spark3/README.md
Outdated
| The `save` method also takes a `SaveMode` option, for which only `SaveMode.Append` is supported. | ||
| The `save` method also takes a `SaveMode` option, it is recommended to use `SaveMode.Append`. | ||
| For maintaining compatibility with source type `"org.apache.phoenix.spark"`, | ||
| `SaveMode.Overwrite` is accepted but it behave same way as `SaveMode.Append`. |
Contributor
There was a problem hiding this comment.
grammar: "behaves the same way"
phoenix5-spark3/README.md
Outdated
| The `"org.apache.phoenix.spark"` datasource does not accept the `"jdbcUrl"` parameter, | ||
| only `"zkUrl"` | ||
| - DataSourceV1 implementation was removed, source type `"org.apache.phoenix.spark"` | ||
| uses DatasourceV2 since connector 6.0.0 release. |
| * The class facilitates the creation of a batch write operation that is configured with the provided | ||
| * logical write information and options specific to the Phoenix data source. | ||
| * | ||
| * Note: Overwrite mode does not do truncate table and behave the same as Append mode. |
Contributor
|
This needs a rebase as I merged PHOENIX-7561 @rejeb . |
Contributor
Author
@stoty, how can I run the checks that Yetus do before pushing to repository. |
Contributor
Not easily. In theory you can also run Yetus locally, but setting that up is a big task, I would not recommend that (unless you plan to work on Yetus support) |
Contributor
Author
Thanks, I'll wait for report instead. |
rejeb
force-pushed
the
PHOENIX-7407
branch
5 times, most recently
from
da4d8ec to
49eef1e
Compare
Contributor
Author
|
I've been unable to fix the spotbugs error. Seams like it is a false positive. |
stoty
reviewed
Jun 2, 2025
| data | ||
| .write | ||
| .format("phoenix") | ||
| .mode(SaveMode.Overwrite) |
Contributor
There was a problem hiding this comment.
Since we don't actually implement Overwrite, wouldn't it make more sense to use Append here ?
Contributor
Author
There was a problem hiding this comment.
This was in spark2 connector, the Append mode is available in spark3.
Would you like to add Append mode support for spark2 ?
If it is the case, I think it is better to do it in a seperate ticket.
Contributor
There was a problem hiding this comment.
Sure, it makes sense to handle that separately.
stoty
reviewed
Jun 2, 2025
| override def createRelation(sqlContext: SQLContext, mode: SaveMode, | ||
| parameters: Map[String, String], data: DataFrame): BaseRelation = { | ||
|
|
||
| if (!mode.equals(SaveMode.Overwrite)) { |
Contributor
There was a problem hiding this comment.
Is there an advatage to keeping this check ?
Do we have tests that check for it ?
Contributor
Author
There was a problem hiding this comment.
The same check is done in spark2 datasource V2 code in this class org.apache.phoenix.spark.datasource.v2.writer.PhoenixDataSourceWriter.
IDK if there is a test for this.
Contributor
|
This mostly looks good, my last concern is whether we should force Overwite in the compatibility layer (as noted above.) |
Contributor
Author
For spark3, SaveMode is forced to Append in the compatibility layer. |
meszinorbi
pushed a commit
to meszinorbi/phoenix-connectors-up
that referenced
this pull request
Jun 13, 2025
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.
Removed datasource V1 classes.
Since "org.apache.phoenix.spark" uses datasource V2 implementation there is no need to keep all tests in
PhoenixSparkDatasourceV1IT. I only kept necessary tests because others are duplication of the ones inPhoenixSparkIT.I added addtional steps in github action to generates tests reports details.