Skip to content

Conversation

@adrian-wang
Copy link
Contributor

This is just another solution to SPARK-3485, in addition to PR #2355
In this patch, we will use ConventionHelper and FunctionRegistry to invoke a simple udf evaluation, which rely more on hive, but much cleaner and safer.
We can discuss which one is better.

@adrian-wang
Copy link
Contributor Author

test this please.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have started for PR 2407 at commit 0d69eb4.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have finished for PR 2407 at commit 0d69eb4.

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

@marmbrus
Copy link
Contributor

We may need to coordinate this with #2241, but otherwise I agree this is much cleaner than before. Thanks for looking into this!

@marmbrus
Copy link
Contributor

Can you maybe change the title to something like ... Use GenericUDFUtils.ConversionHelper for Simple UDF type conversions?

@adrian-wang adrian-wang changed the title [SPARK-3485][SQL] another way to pass to hive simple udf [SPARK-3485][SQL] Use GenericUDFUtils.ConversionHelper for Simple UDF type conversions Sep 17, 2014
@adrian-wang
Copy link
Contributor Author

Thanks for referencing pr 2241 here, while I think this should be reviewed first, and will make 2241 simpler, as well as getting date type support into catalyst earlier.

@adrian-wang
Copy link
Contributor Author

I add a test case to this file, the current master will fail the case because we missed Short in the primitiveTypes here. With this new solution, we can get rid of maintaining of primitiveTypes, and eliminate potential bugs here.

@SparkQA
Copy link

SparkQA commented Sep 17, 2014

QA tests have started for PR 2407 at commit 15762d2.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 17, 2014

QA tests have finished for PR 2407 at commit 15762d2.

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

@chenghao-intel
Copy link
Contributor

+1, LGTM. I think this can go first, as it's simpler and cleaner (maybe fixed some unknown bugs), besides, it blocks the #2344.

@marmbrus
Copy link
Contributor

Thanks! I've merged this to master.

@asfgit asfgit closed this in ba68a51 Sep 19, 2014
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.

4 participants