Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Jan 6, 2016

@viirya viirya changed the title Better support of parentheses in partition by. [SPARK-12577][SQL] Better support of parentheses in partition by and order by clause of window function's over clause Jan 6, 2016
@hvanhovell
Copy link
Contributor

@viirya what is the reason for breaking up the identifiers_parser.g? Shall we - at least - wait with doing until #10583 is in, and do this in a separate ticket?

@hvanhovell
Copy link
Contributor

@viirya what line(s) did you change? It is quite hard to see what has changed.

@viirya
Copy link
Member Author

viirya commented Jan 6, 2016

@hvanhovell I just modified precedenceUnarySuffixExpression. But the generated java file will have "code too large" error. So I need to split it to two files.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hvanhovell I made the change here.

@viirya
Copy link
Member Author

viirya commented Jan 6, 2016

@hvanhovell I am ok to wait until #10583 is in. If you are asking to split identifiers_parser.g to two parts in a separate ticket, I am also ok for that. And doing that should make the change in this PR more clearer. Then this one can wait for it too.

@hvanhovell
Copy link
Contributor

@viirya thanks. Could you also explain why you feel that we need to split up the grammar?

@SparkQA
Copy link

SparkQA commented Jan 6, 2016

Test build #48861 has finished for PR 10620 at commit ae1aa8a.

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

@hvanhovell
Copy link
Contributor

@viirya I missed your comment on splitting the file. That makes perfect sense.

@davies code too large error viirya is hitting, looks similar to the problems you are having in #10622.

@davies
Copy link
Contributor

davies commented Jan 6, 2016

@viirya Since you need to rebase this PR, I will help to create a PR only for splitting IdentifiersParser.g, #10624

@viirya
Copy link
Member Author

viirya commented Jan 7, 2016

@davies thanks and I saw that pr was merged. I will rebase this pr later.

@SparkQA
Copy link

SparkQA commented Jan 7, 2016

Test build #48929 has finished for PR 10620 at commit d0511c0.

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

@hvanhovell
Copy link
Contributor

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this support (a is null) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I think so.

@SparkQA
Copy link

SparkQA commented Jan 9, 2016

Test build #49037 has finished for PR 10620 at commit 119a055.

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

@davies
Copy link
Contributor

davies commented Jan 9, 2016

LGTM, merging into master, thanks!

@asfgit asfgit closed this in 95cd5d9 Jan 9, 2016
@viirya viirya deleted the fix-parentheses branch December 27, 2023 18:18
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.

5 participants