Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Dec 1, 2016

What changes were proposed in this pull request?

This fixes the parser rule to match named expressions, which doesn't work for two reasons:

  1. The name match is not coerced to a regular expression (missing .r)
  2. The surrounding literals are incorrect and attempt to escape a single quote, which is unnecessary

How was this patch tested?

This adds test cases for named expressions using the bracket syntax, including one with quoted spaces.

@rxin
Copy link
Contributor

rxin commented Dec 1, 2016

cc @hvanhovell

@SparkQA
Copy link

SparkQA commented Dec 1, 2016

Test build #69502 has finished for PR 16107 at commit 1c0cc09.

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

@srowen
Copy link
Member

srowen commented Dec 2, 2016

Seems correct to me, yes.

@NathanHowell
Copy link

I wrote the buggy version, doh... but this LGTM. Thanks for fix.

@hvanhovell
Copy link
Contributor

hvanhovell commented Dec 2, 2016

LGTM - merging to master/2.1/2.0. Thanks!

asfgit pushed a commit that referenced this pull request Dec 2, 2016
## What changes were proposed in this pull request?

This fixes the parser rule to match named expressions, which doesn't work for two reasons:
1. The name match is not coerced to a regular expression (missing .r)
2. The surrounding literals are incorrect and attempt to escape a single quote, which is unnecessary

## How was this patch tested?

This adds test cases for named expressions using the bracket syntax, including one with quoted spaces.

Author: Ryan Blue <[email protected]>

Closes #16107 from rdblue/SPARK-18677-fix-json-path.

(cherry picked from commit 4877897)
Signed-off-by: Herman van Hovell <[email protected]>
asfgit pushed a commit that referenced this pull request Dec 2, 2016
## What changes were proposed in this pull request?

This fixes the parser rule to match named expressions, which doesn't work for two reasons:
1. The name match is not coerced to a regular expression (missing .r)
2. The surrounding literals are incorrect and attempt to escape a single quote, which is unnecessary

## How was this patch tested?

This adds test cases for named expressions using the bracket syntax, including one with quoted spaces.

Author: Ryan Blue <[email protected]>

Closes #16107 from rdblue/SPARK-18677-fix-json-path.

(cherry picked from commit 4877897)
Signed-off-by: Herman van Hovell <[email protected]>
@asfgit asfgit closed this in 4877897 Dec 2, 2016
@rdblue
Copy link
Contributor Author

rdblue commented Dec 2, 2016

Thanks for the quick review!

robert3005 pushed a commit to palantir/spark that referenced this pull request Dec 15, 2016
## What changes were proposed in this pull request?

This fixes the parser rule to match named expressions, which doesn't work for two reasons:
1. The name match is not coerced to a regular expression (missing .r)
2. The surrounding literals are incorrect and attempt to escape a single quote, which is unnecessary

## How was this patch tested?

This adds test cases for named expressions using the bracket syntax, including one with quoted spaces.

Author: Ryan Blue <[email protected]>

Closes apache#16107 from rdblue/SPARK-18677-fix-json-path.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
## What changes were proposed in this pull request?

This fixes the parser rule to match named expressions, which doesn't work for two reasons:
1. The name match is not coerced to a regular expression (missing .r)
2. The surrounding literals are incorrect and attempt to escape a single quote, which is unnecessary

## How was this patch tested?

This adds test cases for named expressions using the bracket syntax, including one with quoted spaces.

Author: Ryan Blue <[email protected]>

Closes apache#16107 from rdblue/SPARK-18677-fix-json-path.
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.

6 participants