Add Spark 4.0 support via deequ:2.0.14-spark-4.0#7
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This response was generated using AI and may not be fully accurate. If this doesn't help, please reply and a maintainer will follow up.
Additional feedback:
pydeequ/configs.py:36 — spark_version[:3] truncates to 3 characters, so "4.0" works, but a version like "3.5" also works only by coincidence. The lookup should use the full spark_version string (which is already major.minor from _extract_major_minor_versions), not a redundant slice. This is a pre-existing bug but the new "4.0" entry makes it more visible — if someone sets SPARK_VERSION=4.0.0, _extract_major_minor_versions returns "4.0", and "4.0"[:3] gives "4.0" which works, but for any future 2-digit minor (e.g. "3.10"), [:3] would produce "3.1" — wrong mapping. Use spark_version directly instead of spark_version[:3].
| Scala sequence | ||
| """ | ||
| return jvm.scala.collection.JavaConversions.iterableAsScalaIterable(iterable).toSeq() | ||
| return jvm.scala.collection.JavaConverters.iterableAsScalaIterableConverter(iterable).asScala().toSeq() |
There was a problem hiding this comment.
JavaConverters.iterableAsScalaIterableConverter was deprecated in Scala 2.13 and removed in Scala 2.13.13+. Spark 4.0 uses Scala 2.13. The correct Scala 2.13+ API is scala.jdk.CollectionConverters, not scala.collection.JavaConverters. This will break on the very Spark 4.0 target this PR adds. Similarly, JavaConverters.mapAsJavaMapConverter on lines 108 and 112 has the same problem. You need to branch on Scala version (or Spark version) and use scala.jdk.CollectionConverters for Spark 4.0 / Scala 2.13.
| """ | ||
| self._run_result = run | ||
| profile_map = self._jvm.scala.collection.JavaConversions.mapAsJavaMap(run.profiles()) # TODO from ScalaUtils | ||
| profile_map = self._jvm.scala.collection.JavaConverters.mapAsJavaMapConverter(run.profiles()).asJava() # TODO from ScalaUtils |
There was a problem hiding this comment.
Same issue: scala.collection.JavaConverters.mapAsJavaMapConverter is deprecated/removed in Scala 2.13.13+. This will fail on Spark 4.0. Needs the same version-conditional fix as scala_utils.py.
| java-version: ${{matrix.JAVA_VERSION}} | ||
|
|
||
| - name: Running tests with pyspark==${{matrix.PYSPARK_VERSION}} | ||
| env: |
There was a problem hiding this comment.
poetry run pip install pyspark==$SPARK_VERSION bypasses Poetry's dependency resolver and can create an inconsistent environment (Poetry won't know about the installed version). The previous poetry add pyspark==$SPARK_VERSION was intentional. If poetry add doesn't work for Spark 4.0 due to the version constraint, the constraint in pyproject.toml should be sufficient — but mixing poetry run pip install with Poetry-managed deps is fragile.
| @@ -0,0 +1,121 @@ | |||
| name: PyDeequ Bot | |||
There was a problem hiding this comment.
This workflow grants issues: write and pull-requests: write permissions and executes Python scripts that read secrets (AWS credentials, Slack webhook, GitHub token) and invoke external services (Bedrock, S3). The analyze job runs on PRs from any contributor (pull_request: [opened, reopened, synchronize]), and while there's a fork check (github.event.pull_request.head.repo.full_name == github.repository), the issue_comment trigger has no such guard — any user comment on any issue triggers the full pipeline including AWS credential assumption. This is a significant security surface that should be reviewed carefully.
| fixed = [] | ||
| for line in lines: | ||
| if line.strip().startswith("```"): | ||
| in_code_block = not in_code_block |
There was a problem hiding this comment.
_fix_accidental_issue_refs replaces all #N patterns outside code blocks with option N, which will mangle legitimate markdown headings (e.g., ## Heading won't match, but #1 priority would become option 1 priority). More importantly, it will corrupt code references like pydeequ issue #123 in the bot's own responses. The regex r'#(\d+)(?!\w)' is too broad.
| @@ -0,0 +1,555 @@ | |||
| """ | |||
There was a problem hiding this comment.
This 555-line file containing the entire bot orchestration logic is included in a PR titled "Add Spark 4.0 support". The bot code is unrelated to Spark 4.0 compatibility and should be in a separate PR to keep changes reviewable and bisectable.
There was a problem hiding this comment.
This response was generated using AI and may not be fully accurate. If this doesn't help, please reply and a maintainer will follow up.
Additional feedback:
pydeequ/configs.py:38 — spark_version[:3] truncates to 3 characters, so "4.0" works, but a version like "3.10" (hypothetical) would be truncated to "3.1". More importantly, _get_spark_version() already returns the major.minor string via _extract_major_minor_versions, so the [:3] slice is redundant and potentially lossy. Use spark_version directly as the dict key instead of spark_version[:3].
| numpy = ">=1.14.1" | ||
| pandas = ">=0.23.0" | ||
| pyspark = { version = ">=2.4.7,<3.4.0", optional = true } | ||
| pyspark = [ |
There was a problem hiding this comment.
The pyspark dependency for Python >=3.9 allows <5.0.0, but the Python >=3.8,<3.9 variant caps at <4.0.0. This means Python 3.8 users silently get no PySpark 3.5 support if 3.5 happens to be >=4.0 in the future. More critically, both variants are optional = true but there's no mechanism to ensure only one is selected — Poetry may struggle with two entries for the same optional dependency differentiated only by python version markers. Consider testing that poetry lock resolves correctly for both Python version ranges.
| def empty_scala_seq(jvm): | ||
| """ | ||
| Returns an empty Scala immutable List (Nil), usable as Seq[_]. | ||
| Uses JavaConverters.toList() to produce an immutable.List rather than |
There was a problem hiding this comment.
empty_scala_seq creates a new ArrayList and converts it on every call. This is used in hot paths (e.g., inside satisfies, Compliance). Consider caching the result or using a simpler approach like jvm.scala.collection.immutable.Nil$.MODULE$ which directly returns the Scala Nil singleton without going through Java conversion.
|
|
||
| - name: Running tests with pyspark==${{matrix.PYSPARK_VERSION}} | ||
| env: | ||
| SPARK_VERSION: ${{matrix.PYSPARK_VERSION}} |
There was a problem hiding this comment.
The if [ -n "$PANDAS_VERSION" ] guard means only the Spark 4.0 matrix entry gets a specific pandas version. For other matrix entries, PANDAS_VERSION is undefined, so they get whatever pandas Poetry resolves. This is fine now, but the variable name is misleading — it's actually a version specifier (e.g., >=2.0.0), not a version. Consider renaming to PANDAS_VERSION_SPEC for clarity.
| steps: | ||
| - uses: actions/checkout@v3 | ||
|
|
||
| - uses: actions/setup-python@v2 |
There was a problem hiding this comment.
The Spark 4.0 matrix entry uses JAVA_VERSION: "17" but actions/setup-java@v1 is very outdated and may not support Java 17 well. Consider upgrading to actions/setup-java@v3 (which is also consistent with actions/checkout@v3 already used). Similarly, actions/setup-python@v2 should be upgraded to v4 or v5.
1 participant
Issue #, if available:
Description of changes:
Adds Spark 4.0 compatibility by migrating from deprecated JavaConversions to JavaConverters, adding empty_scala_seq helper, and updating configs/CI matrix.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.