Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OPS-5046 support spark 3.4.0 on SPOK #98

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

xuzeng012
Copy link
Contributor

@xuzeng012 xuzeng012 commented Jun 21, 2023

spark 3.4.0 made a change to spark type CharType which requires the constructor to take a parameter length so we are using spark's internal method to take care of all that parsing

sparkly/utils.py Outdated Show resolved Hide resolved
@xuzeng012 xuzeng012 force-pushed the OPS-5046-support-spark-3.4.0-on-SPOK branch from dc25d08 to 8abe6fc Compare June 22, 2023 00:06
sparkly/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@srstrickland srstrickland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are broken, it seems because the Dockerfile is not properly set up to install jdk8. this needs to be fixed. I recommend testing locally using make test, or for faster iteration, make dev then run tests with tox when you're inside the container.

@xuzeng012 xuzeng012 force-pushed the OPS-5046-support-spark-3.4.0-on-SPOK branch from 8abe6fc to f1dd33a Compare June 22, 2023 00:26
@srstrickland
Copy link
Contributor

Looks good, but let's make sure! We can add a spark34 section to tox.ini to force our test suite to run on spark 3.4. just copy the spark33 section and modify the pyspark version.

@xuzeng012 xuzeng012 force-pushed the OPS-5046-support-spark-3.4.0-on-SPOK branch from 9db2d1d to 411d5af Compare June 22, 2023 00:53
@xuzeng012 xuzeng012 force-pushed the OPS-5046-support-spark-3.4.0-on-SPOK branch from 411d5af to d186b08 Compare June 22, 2023 00:54
@srstrickland
Copy link
Contributor

srstrickland commented Jun 22, 2023

Looks like there are several failures with 3.4.0:

tests/integration/test_catalog.py ..............F...............F.       [ 19%]
tests/integration/test_functions.py .....................                [ 31%]
tests/integration/test_instant_testing.py ..                             [ 32%]
tests/integration/test_reader.py ......                                  [ 36%]
tests/integration/test_session.py ....                                   [ 38%]
tests/integration/test_testing.py .........                              [ 44%]
tests/integration/test_writer.py .....FF.....................            [ 61%]
tests/unit/test_instant_testing.py ....                                  [ 63%]
tests/unit/test_reader.py ...........                                    [ 70%]
tests/unit/test_session.py ...............                               [ 79%]
tests/unit/test_testing.py ..........                                    [ 85%]
tests/unit/test_utils.py .................                               [ 95%]
tests/unit/test_writer.py ........                                       [100%]

FAILED tests/integration/test_catalog.py::TestSparklyCatalog::test_rename_table_non_default_db
FAILED tests/integration/test_catalog.py::TestSparklyWithOldCatalog::test_rename_table_non_default_db
FAILED tests/integration/test_writer.py::TestWriteKafka::test_write_kafka_dataframe
FAILED tests/integration/test_writer.py::TestWriteKafka::test_write_kafka_dataframe_error

it would seem that more work is required here to make everything work.

@xuzeng012 xuzeng012 force-pushed the OPS-5046-support-spark-3.4.0-on-SPOK branch 2 times, most recently from aeec316 to a6a47bf Compare June 22, 2023 23:55
tests/integration/base.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@xuzeng012 xuzeng012 force-pushed the OPS-5046-support-spark-3.4.0-on-SPOK branch from a6a47bf to f5a8595 Compare June 23, 2023 22:03
@xuzeng012 xuzeng012 merged commit 393d134 into master Jun 26, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants