-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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
fix: Allow multiple EXTRACTJSONFIELD invocations on different paths (#8122) #8123
fix: Allow multiple EXTRACTJSONFIELD invocations on different paths (#8122) #8123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good.
+1
final JsonPathTokenizer tokenizer = new JsonPathTokenizer(path); | ||
final List<String> tokens = ImmutableList.copyOf(tokenizer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be good to keep a the tokens
initialized once in case the path
is equals to the latest path. What about adding a new class variable to keep the latest path seen, and check if the new path is equals to the latest path, if true, then re-use the current tokens
class variable, if not, then initialize it with a new JsonPathTokenizer and List objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for the feedback. I will cover your points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
323e1a9
to
105f448
Compare
@stefanfrehse I created a PR for testing your PR here - #8152. There are some limitations for allowing testing external contributions. I'll merge this PR once the tests pass. If they do not pass, could you check what needs to be fixed? |
It was running locally fine. Do I see as an external contributor why the build fails? |
@stefanfrehse nvm, I see your job run, but failed. In the past, I've seen issue with contributors PRs not running. But this one just run, so you can take a look at the issue. |
105f448
to
6980364
Compare
@spena build fixed. Thanks to the static analysis, bug has been detected and is fixed now. |
I merged the PR. Thanks @stefanfrehse for your contribution. |
Description
Allows the
extract
function to be invoked on the same instance multiple times with differentpath
variables.Testing done
A unit test has been added and the steps that reproduce the bug validated.
Reviewer checklist