Skip to content
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

Record accessor helper #1637

Merged
merged 8 commits into from
Jul 28, 2017
Merged

Record accessor helper #1637

merged 8 commits into from
Jul 28, 2017

Conversation

repeatedly
Copy link
Member

Implement record_accessor helper for nested record support.
Support two jsonpath like syntaxes.

  • dot notation: $.key1.key2[0]
  • bracket notation: $['key1'][0]['this.is.key2']
  • single key: key

In this patch, I also update filter_grep to use this helper.

NOTE: I implement Accessor with normal way. If need the performance, I will change its implementaion to eval way: https://gist.github.com/repeatedly/ab553ed260cd080bd01ec71da9427312

@repeatedly repeatedly added feature request *Deprecated Label* Use enhancement label in general v0.14 labels Jul 19, 2017
@repeatedly repeatedly self-assigned this Jul 19, 2017
@repeatedly repeatedly requested a review from mururu July 19, 2017 12:47
@repeatedly
Copy link
Member Author

@mururu Could you check this patch?

@repeatedly
Copy link
Member Author

Bench result:

% ruby dig_bench.rb
       user     system      total        real
baseline     0.570000   0.000000   0.570000 (  0.573822)
expand args  1.100000   0.000000   1.100000 (  1.093701)
pre-eval     0.860000   0.000000   0.860000 (  0.867493)

@sonots
Copy link
Member

sonots commented Jul 19, 2017

I thought jsonpath supports $.key1[0]['this.is.key2'], but this patch does not?

@repeatedly
Copy link
Member Author

@sonots yes. In fluentd case, mix case seems no merits.

@sonots
Copy link
Member

sonots commented Jul 19, 2017

Well, I feel allowing users to write $.key1[0]['this.is.key2'] is a merit. As a user, I want to use bracket notation for only part having . (this.is.key2).

I agree that it makes parsing a bit complex and consumes time, but I think it is acceptable tradeoff because parser usually runs only once at "configure".

@repeatedly
Copy link
Member Author

I think it is acceptable tradeoff because parser usually runs only once at "configure".

Yes, it doesn't have a impact at runtime.

This is just jsonpath like, not jsonpath, so I don't think following similar syntax is not must. For documentation and maintainance, separating two way is less cost at the baseline.

@sonots
Copy link
Member

sonots commented Jul 19, 2017

We need to get users' opinions. As one user, I like to get allowed $.key1[0]['this.is.key2'].

assert_equal ['key1', 'key2', 0], result
end

data('dot' => '$.key1[0].ke y2',
Copy link
Member

Choose a reason for hiding this comment

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

Wow, this is acceptable? It looks wierd for me. I think we should not allow it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

@sonots sonots Jul 19, 2017

Choose a reason for hiding this comment

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

I think typical jsonpath parser does not accept this. At least https://github.com/json-path/JsonPath did not accept this.

Copy link
Member Author

@repeatedly repeatedly Jul 20, 2017

Choose a reason for hiding this comment

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

It seems javascript object limitation. javascript object field can't contain space.
But fluentd target is an event record, so there is no such limitation.

Copy link
Member

Choose a reason for hiding this comment

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

I am against to extend specifications because it makes difficult to refactor implementation later (for example, think of changing jsonpath parser to 3rd party implementation later) without breaking backward compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have a plan to use 3rd party jsonpath parser because this is just jsonpath like, not jsonpath. jsonpath has lots of useless feature in fluentd, e.g.g filter, wildcard and etc.
So we want limited well-known syntax, not entire jsonpath.

Copy link
Member Author

Choose a reason for hiding this comment

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

But limiting several characters in dot notation is not bad idea because bracket notation is straight way for such cases.

Copy link
Member

Choose a reason for hiding this comment

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

I don't strongly oppose this, but it just looks weird for me too.

@mururu
Copy link
Member

mururu commented Jul 20, 2017

$.hello.[0] is parsed as ["hello", "[0]", 0]. It should be parsed as ["hello", 0] or ["hello", "[0]"] or just raise error.
https://github.com/joshbuddy/jsonpath parses it as ["hello", 0]. I haven't check other implementations.

@mururu
Copy link
Member

mururu commented Jul 20, 2017

Minor comment: When parsing $[] or $.hello[], it says invalid value for Integer(): "]", but it is better to say invalid value for Integer(): "" .

@repeatedly
Copy link
Member Author

@mururu Good catch. I will add more validation.

@repeatedly
Copy link
Member Author

repeatedly commented Jul 26, 2017

Applied reviews and improve the performance for single key.
PTAL

Copy link
Member

@mururu mururu left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request *Deprecated Label* Use enhancement label in general v0.14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants