Pom refactor - Add Spark 3.5 connector#152
Conversation
…vider acceptance error
…endencies between Spark versions.
Summary of ChangesHello @stevelordbq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the project's build system to improve modularity and facilitate multi-version Spark support. By introducing dedicated core and common Spark modules, it streamlines the process of adding new Spark connector versions. The primary outcome is the successful integration and support for Spark 3.5, complete with updated documentation and comprehensive test coverage. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring of the project's build structure to support multiple Spark versions from a single codebase, including the addition of support for Spark 3.5. The changes introduce a modular approach with spanner-core, spanner-spark-common, and version-specific modules, which is a solid strategy. My review focuses on improving maintainability by reducing code duplication in both the build configuration and the source code, fixing some minor issues like wildcard imports and invalid SQL in test files, and suggesting best practices for logging and code formatting enforcement.
# Conflicts: # spark-3.1-spanner-lib/src/main/java/com/google/cloud/spark/spanner/Spark31SpannerTableProvider.java
|
/gcbrun |
|
/gcbrun |
jiangzzhu
left a comment
There was a problem hiding this comment.
Could you also update https://github.com/GoogleCloudDataproc/spark-spanner-connector/blob/main/.github/workflows/dev-ci.yml to build 3.5 as well? thanks!
And also |
| | spark-3.2-spanner | | | | ✓ | ✓ | ✓ | ✓ | | ✓ | | | ||
| | spark-3.3-spanner | | | | ✓ | ✓ | ✓ | ✓ | | ✓ | | | ||
| | spark-3.5-spanner | | | | | | ✓ | | ✓ | | ✓ | | ||
| | Connector \ Dataproc Image | 1.3 | 1.4 | 1.5 | 2.0 | 2.1 | 2.2 | Serverless<br>Image 1.1 | Serverless<br>Image 1.2 | Serverless<br>Image 2.0 | Serverless<br>Image 2.1 | Serverless<br>Image 2.2 | |
There was a problem hiding this comment.
Why does spark 3.1,3.2,3.3 not work with serverless 1.2 and 2.2?
There was a problem hiding this comment.
We haven't tested that those combinations work. So it may work.
https://docs.cloud.google.com/dataproc-serverless/docs/concepts/versions/spark-runtime-1.2
Did you want us to increase the cross version compatibility testing?
There was a problem hiding this comment.
If adding the test is simple (by acceptance test), maybe we could add it, so we could check on those entries.
Or we could say it is supported but not tested, to avoid confusion that it is not supported. What do you think?
|
A few questions on the current PR.
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:testCompile (default-testCompile) on project spark-3.3-spanner-lib: Compilation failure Could you fix this? Otherwise, the release process will fail. Thanks!
[ERROR] WriteIntegrationTest.testEmptyDataFrameWrite:140 » SparkClassNotFound [DATA_SOURCE_NOT_FOUND] Failed to find the data source: cloud-spanner. Please find packages at Please also take a look at this. It also breaks the release process. Thanks!
|
Please note @jiangzzhu , now we have separate builds for each Spark version, it doesn't make sense to aggregate the profiles together, since that would use the Spark version set by the last profile, which would be Spark 3.3 or 3.5 in your examples above. However, I will investigate this failure for a single Spark version. |
Creates a separate build for each Spark version.
The approach was not chosen, however Jiang asked that we preserve this branch in case it is adopted later.