Skip to content

Conversation

@HyukjinKwon
Copy link
Member

This PR is followed by #8391.
Previous PR fixes JDBCRDD to support null-safe equality comparison for JDBC datasource. This PR fixes the problem that it can actually return null as a result of the comparison resulting error as using the value of that comparison.

@JoshRosen
Copy link
Contributor

Please update the pull request description to describe the changes in this patch.

@JoshRosen
Copy link
Contributor

Will this case be covered by the end-to-end docker tests?

@HyukjinKwon
Copy link
Member Author

Should I better write some test codes for this separately?

@yhuai
Copy link
Contributor

yhuai commented Nov 18, 2015

Can we add a test?

@HyukjinKwon
Copy link
Member Author

I added a simple test for this. I wanted to add a test including null but when the given value is null, Spark converts it to IsNull. To make this worse, more complicated logics cannot be applied here to test null-safety properly since now filters are pushed down only for basic operations (not even cast or arithmetic calculations). For now, I think this is the best to test this.

@HyukjinKwon
Copy link
Member Author

test this please

@HyukjinKwon
Copy link
Member Author

It looks Jenkins does not run the test for the past commits that I made as a user not added to whitelist. Would anybody run the test for this please if it looks good?

@yhuai
Copy link
Contributor

yhuai commented Nov 20, 2015

test this please

@SparkQA
Copy link

SparkQA commented Nov 20, 2015

Test build #46425 has finished for PR 8743 at commit bf0b22c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

Just a question. Now it looks the PR for the end-to-end docker tests is merged. Do you think it needs all the tests for all the databases (namely MySQLIntegrationSuite and PostgresIntegrationSuite for now)?

@rxin
Copy link
Contributor

rxin commented Dec 30, 2015

@zsxwing can you review this one?

@maropu
Copy link
Member

maropu commented Dec 30, 2015

@HyukjinKwon +1, I think it'd better to add tests in MySQLIntegrationSuite and PostgresIntegrationSuite.

@HyukjinKwon
Copy link
Member Author

Hm.. actually I think adding tests more with the docker ingeration tests is a bit over-tested. It looks the comparison operators I used are all already being used in compileFilter and I think the logical operators can be covered in the existing test.

@maropu
Copy link
Member

maropu commented Dec 30, 2015

Yes and you're right. We need to clearly define the rule that decides which tests should be added in docker-integration-tests.

Copy link
Member

Choose a reason for hiding this comment

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

It took me several minutes to understand it. Maybe (attr IS NULL AND value IS NULL) OR (attr <> null AND value <> null AND attr = value) is easier.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, my previous expr doesn't work.

@zsxwing
Copy link
Member

zsxwing commented Dec 31, 2015

I'm wondering if it's okey to add sealed to Filter and remove case _. If so, the compiler can help us find such issue.

@HyukjinKwon
Copy link
Member Author

@zsxwing I will anyway resolve the conflicts first

@rxin
Copy link
Contributor

rxin commented Jan 1, 2016

Sorry I kept merging pull requests that made your life harder. Can you bring it up to date again? I promise this is the last one.

Conflicts:
	sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala
@HyukjinKwon
Copy link
Member Author

@rxin Nothing is easy and happy new year:) I just resolved conflicts.

@HyukjinKwon
Copy link
Member Author

test this please

@SparkQA
Copy link

SparkQA commented Jan 1, 2016

Test build #2287 has finished for PR 8743 at commit 17b6953.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

test this please

@SparkQA
Copy link

SparkQA commented Jan 2, 2016

Test build #2291 has finished for PR 8743 at commit a489a33.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 2, 2016

Test build #2294 has finished for PR 8743 at commit a489a33.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

test this please

@SparkQA
Copy link

SparkQA commented Jan 2, 2016

Test build #2295 has finished for PR 8743 at commit a489a33.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Jan 2, 2016

Thanks - I've merged this.

@asfgit asfgit closed this in 94f7a12 Jan 2, 2016
zzcclp added a commit to zzcclp/spark that referenced this pull request Jul 27, 2016
@HyukjinKwon HyukjinKwon deleted the SPARK-10180 branch September 23, 2016 18:28
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.

7 participants