Navigation Menu

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

[SPARK-22726] [TEST] Basic tests for Binary Comparison and ImplicitTypeCasts #19918

Closed
wants to merge 1 commit into from

Conversation

gatorsmile
Copy link
Member

What changes were proposed in this pull request?

Before we deliver the Hive compatibility mode, we plan to write a set of test cases that can be easily run in both Spark and Hive sides. We can easily compare whether they are the same or not. When new typeCoercion rules are added, we also can easily track the changes. These test cases can also be backported to the previous Spark versions for determining the changes we made.

This PR is the first attempt for improving the test coverage for type coercion compatibility. We generate these test cases for our binary comparison and ImplicitTypeCasts based on the Apache Derby test cases in https://github.com/apache/derby/blob/10.14/java/testing/org/apache/derbyTesting/functionTests/tests/lang/implicitConversions.sql

How was this patch tested?

N/A

-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the test cases are based on Apache Derby, I keep Apache license here.

@@ -1,5 +1,5 @@
-- Automatically generated by SQLQueryTestSuite
-- Number of queries: 31
-- Number of queries: 32
Copy link
Member Author

@gatorsmile gatorsmile Dec 7, 2017

Choose a reason for hiding this comment

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

It sounds like the previous PR did not use our SPARK_GENERATE_GOLDEN_FILES to generate the result file.

@@ -300,7 +300,7 @@ class SQLQueryTestSuite extends QueryTest with SharedSQLContext {
Locale.setDefault(originalLocale)

// For debugging dump some statistics about how much time was spent in various optimizer rules
logInfo(RuleExecutor.dumpTimeSpent())
logWarning(RuleExecutor.dumpTimeSpent())
Copy link
Member Author

Choose a reason for hiding this comment

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

I have to change it to logWarning; otherwise, the time for each rule is not shown in the test result log. cc @cloud-fan

@gatorsmile
Copy link
Member Author

cc @cloud-fan

@wangyum This is just an example. You can follow it to add more test cases for each type coercion rule. Later, we need to run the same SQLs in the Hive side too.

@SparkQA
Copy link

SparkQA commented Dec 7, 2017

Test build #84597 has finished for PR 19918 at commit 5451cf5.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.


CREATE TEMPORARY VIEW t AS SELECT 1;

SELECT cast(1 as binary) = '1' FROM t;
Copy link
Member

Choose a reason for hiding this comment

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

Seems binary comparison without <=>.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea <=> is missed. Do Derby tests also miss it?

Copy link
Contributor

Choose a reason for hiding this comment

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

might be OK as <=> should have same type coercion rule as =

Copy link
Member Author

Choose a reason for hiding this comment

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

=== is consistent with <=>

I think we can skip it. Then, we can run the whole suite to Hive with minor changes.

@cloud-fan
Copy link
Contributor

LGTM

@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Dec 11, 2017

Test build #84719 has finished for PR 19918 at commit 5451cf5.

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

@gatorsmile
Copy link
Member Author

retest this please

1 similar comment
@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Dec 11, 2017

Test build #84727 has finished for PR 19918 at commit 5451cf5.

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

@SparkQA
Copy link

SparkQA commented Dec 11, 2017

Test build #84729 has finished for PR 19918 at commit 5451cf5.

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

@gatorsmile
Copy link
Member Author

Since the failure in SparkR is not related to this PR, I will merge this.

Thanks! Merged to master.

@asfgit asfgit closed this in 3d82f6e Dec 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants