Skip to content

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Nov 2, 2019

What changes were proposed in this pull request?

Support non-reversed keywords to be used in high order functions.

Why are the changes needed?

the keywords are non-reversed.

Does this PR introduce any user-facing change?

yes, all non-reversed keywords can be used in high order function correctly

How was this patch tested?

add uts

@yaooqinn
Copy link
Member Author

yaooqinn commented Nov 2, 2019

cc @ueshin @gatorsmile @cloud-fan @HyukjinKwon @dongjoon-hyun, thanks in advance.

@SparkQA
Copy link

SparkQA commented Nov 2, 2019

Test build #113126 has finished for PR 26366 at commit 2895b3b.

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

@yaooqinn
Copy link
Member Author

yaooqinn commented Nov 2, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Nov 2, 2019

Test build #113133 has finished for PR 26366 at commit 2895b3b.

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

@yaooqinn
Copy link
Member Author

yaooqinn commented Nov 3, 2019

retest this please

| IDENTIFIER '->' expression #lambda
| '(' IDENTIFIER (',' IDENTIFIER)+ ')' '->' expression #lambda
| identifier '->' expression #lambda
| '(' identifier (',' identifier)+ ')' '->' expression #lambda
Copy link
Member

Choose a reason for hiding this comment

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

Ur, I see. Looks reasonable to me.

-- Transform an array with index
select transform(ys, (y, i) -> y + i) as v from nested;
-- use non reversed keywords
select transform(ys, (cost, i) -> cost + i) as v from nested;
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the two tests above into the end of the file to make diff smaller in sq.out.

Copy link
Member

Choose a reason for hiding this comment

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

Also, can you add some tests for error cases if ansi=true?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for you suggestion. I just moved them to the end of this file, and add some cases to verify with ansi flag.

@SparkQA
Copy link

SparkQA commented Nov 3, 2019

Test build #113148 has finished for PR 26366 at commit 2895b3b.

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

@SparkQA
Copy link

SparkQA commented Nov 3, 2019

Test build #113163 has finished for PR 26366 at commit 5db6d6d.

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

@yaooqinn yaooqinn requested a review from maropu November 4, 2019 03:47
Copy link
Member

@maropu maropu left a comment

Choose a reason for hiding this comment

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

@HyukjinKwon
Copy link
Member

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants