Skip to content

[SPARK-3485][SQL] should check parameter type when look for constructors #2355

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.

Sign up for GitHub

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

Jump to bottom
Closed
adrian-wang wants to merge 5 commits into from
Closed

[SPARK-3485][SQL] should check parameter type when look for constructors #2355

adrian-wang wants to merge 5 commits into from

Conversation

@adrian-wang
Copy link
Contributor

@adrian-wang adrian-wang commented Sep 11, 2014

In hiveUdfs, we get constructors of primitivetypes by find a constructor which takes only one parameter. This is very dangerous when more than one constructors match. When the sequence of primitiveTypes becomes larger, the problem would occur, causing a exception as

java.lang.IllegalArgumentException: argument type mismatch

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have started for PR 2355 at commit fa68a11.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have finished for PR 2355 at commit fa68a11.

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

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 11, 2014

failure is in streaming.
Actually this is a potential problem if hive decide to add a constructor for timstampwritable with one parameters exactly, other than java.sql.Timestamp. Seems jvm will get a random constructor then.

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 11, 2014

retest this please.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have started for PR 2355 at commit fa68a11.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have finished for PR 2355 at commit fa68a11.

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

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 11, 2014

The last failure was recovery with file input stream in CheckpointSuite and this time, the failure is in block generator throttling in NetworkReceiverSuite.

@adrian-wang adrian-wang mentioned this pull request Sep 12, 2014
Closed
@SparkQA
Copy link

SparkQA commented Sep 14, 2014

QA tests have started for PR 2355 at commit fa68a11.

  • This patch does not merge cleanly!

@SparkQA
Copy link

SparkQA commented Sep 14, 2014

QA tests have finished for PR 2355 at commit fa68a11.

  • This patch passes unit tests.
  • This patch does not merge cleanly!

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 14, 2014

Have my code rebased, should be able to merge cleanly!

@SparkQA
Copy link

SparkQA commented Sep 14, 2014

QA tests have started for PR 2355 at commit 78b4c03.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 14, 2014

QA tests have finished for PR 2355 at commit 78b4c03.

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

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 15, 2014

I have changed the way to fix the problem here, keeping most of the original logics. @marmbrus

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 15, 2014

retest this please.

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have started for PR 2355 at commit 0142696.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have finished for PR 2355 at commit 0142696.

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

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 15, 2014

Sorry for the missings here...

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 15, 2014

@chenghao-intel This is not so complex since it is not GenericUDF, but simple UDF with limited types. So we do not need to call those here.

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have started for PR 2355 at commit 5f25ca5.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have finished for PR 2355 at commit 5f25ca5.

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

@adrian-wang adrian-wang reopened this Sep 16, 2014
@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 16, 2014

retest this please.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have started for PR 2355 at commit 5cddda8.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have finished for PR 2355 at commit 5cddda8.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class ArrayConstructor extends net.razorvine.pickle.objects.ArrayConstructor
    • class SCCallSiteSync(object):

liancheng
liancheng reviewed Sep 16, 2014
View reviewed changes
sql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUdfs.scala Outdated
Copy link
Contributor

@liancheng liancheng Sep 16, 2014

Choose a reason for hiding this comment

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

(a: Any) => wrap(a) should be enough here.

@adrian-wang adrian-wang mentioned this pull request Sep 16, 2014
Closed
@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 16, 2014

Hi @liancheng I have created another PR(PR #2407 ) on this issue, offering another solution. Please let me know which solution do you guys prefer here.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have started for PR 2355 at commit 9b62158.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have finished for PR 2355 at commit 9b62158.

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

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have started for PR 2355 at commit 6cc24bd.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have finished for PR 2355 at commit 6cc24bd.

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

@adrian-wang
Copy link
Contributor Author

adrian-wang commented Sep 17, 2014

maybe 2407 is better, I'll close this.

asfgit pushed a commit that referenced this pull request Sep 19, 2014
@adrian-wang @marmbrus
[SPARK-3485][SQL] Use GenericUDFUtils.ConversionHelper for Simple UDF…
ba68a51 
… type conversions

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.

Author: Daoyuan Wang <daoyuan.wang@intel.com>

Closes #2407 from adrian-wang/simpleudf and squashes the following commits:

15762d2 [Daoyuan Wang] add posmod test which would fail the test but now ok
0d69eb4 [Daoyuan Wang] another way to pass to hive simple udf
@adrian-wang adrian-wang deleted the checkpara branch September 24, 2014 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adrian-wang @SparkQA @liancheng