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

JSONSelection refactoring, $variable syntax, and an extensive new README.md #5142

Merged
merged 12 commits into from May 13, 2024

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented May 10, 2024

Now that the implementation of the JSONSelection syntax lives in this repository, I've taken the time to reorganize the single selection_parser.rs file into a directory called json_selection containing a mod.rs file, with the various components are split out into separate files.

Most importantly, I've also written almost 4500 words in a comprehensive new README.md that lives in the json_selection directory. Please make time to read this documentation to understand the current version of the syntax.

As explained in README.md, I added support for $variable expressions, which will eventually support the $this and $args namespaces, as well as a special $ variable that always refers to the current value being processed. The README.md also describes -> method syntax, which will be useful for arbitrary inline data transformations, but I have not yet added support for that feature to the implementation. The JSLiteral and related grammar rules are only used with -> method syntax, so you can ignore them for now.

An important change since the previous version of the JSONSelection syntax is that leading . characters are no longer required for PathSelection chains. This means you can write post.author.name instead of .post.author.name (though the latter is still supported for backwards compatibility). When you need to express a PathSelection consisting of a single key, you can now write $.data instead of .data, taking advantage of the new $ variable.

Now that this code lives in the same repository with other code that uses it, I was able to update all existing consumption sites to accommodate my various renamings (like Selection to JSONSelection, and Property to Key), but if you have outstanding PRs or branches that you're working on, you may encounter some minor rebase conflicts. Let me know if anything I've changed is confusing.

@router-perf
Copy link

router-perf bot commented May 10, 2024

CI performance tests

  • step - Basic stress test that steps up the number of users over time
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • large-request - Stress test with a 1 MB request payload
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • xxlarge-request - Stress test with 100 MB request payload
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • xlarge-request - Stress test with 10 MB request payload
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • no-graphos - Basic stress test, no GraphOS.
  • reload - Reload test over a long period of time at a constant rate of users
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • const - Basic stress test that runs with a constant number of users

@benjamn benjamn requested a review from timbotnik May 10, 2024 17:30
Copy link
Contributor

@lennyburdette lennyburdette left a comment

Choose a reason for hiding this comment

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

this is awesome!

quick non-blocking Q: $args and $this are not "special" — you can use any identifier in a VarPath, right? i think that's the right move, and we'll have semantic validation that disallows things like $foo in a different part of the code?

@benjamn
Copy link
Member Author

benjamn commented May 13, 2024

@lennyburdette Yep, $args and $this are not special, and we still need to write code that passes them into the ApplyTo::apply_with_vars method. The only special (always defined) variable at the moment is $.

As for validation, referring to undefined variables (or undefined paths like $var.does.not.exist) currently produces a runtime ApplyToError, which is non-fatal for the rest of the selection (similar to GraphQL result.errors). If the nonexistent value was the immediate child of an object property, the output object simply won't have that property, but if it was an array element, we need a placeholder value to preserve the array structure, so the value becomes null.

@benjamn benjamn force-pushed the benjamn/json_selection-refactoring branch from 9adc0f6 to 42a076c Compare May 13, 2024 14:07
@lennyburdette
Copy link
Contributor

Cool, that makes sense. It'll be fun for us (@dylan-apollo) to make static validation match the ApplyTo runtime behavior

@benjamn benjamn merged commit 17ed310 into connectors May 13, 2024
10 of 12 checks passed
@benjamn benjamn deleted the benjamn/json_selection-refactoring branch May 13, 2024 16:06
@benjamn
Copy link
Member Author

benjamn commented May 13, 2024

I went ahead and merged this after @lennyburdette's review, since it's vulnerable to merge conflicts and I've been rebasing frequently. I don't mean to shut down discussion or comments from the other reviewers, or anyone who has thoughts/questions.

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.

None yet

2 participants