-
Notifications
You must be signed in to change notification settings - Fork 85
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
PgQuery::Node: Add inner and inner= helpers to modify inner object #306
Conversation
7fa8700
to
cc7d11c
Compare
lib/pg_query/node.rb
Outdated
@@ -1,22 +1,27 @@ | |||
module PgQuery | |||
# Patch the auto-generated generic node type with additional convenience functions | |||
class Node | |||
def self.inner_class_to_name(klass) | |||
@inner_class_to_name ||= descriptor.lookup_oneof('node').map { |f| [f.subtype.msgclass, f.name.to_sym] }.to_h |
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.
https://bugs.ruby-lang.org/issues/15143
Can we use to_h with a block here? That was merged in 2018.
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.
Good point - looks like that was added in Ruby 2.6 (see commit), so we could use it.
Because of how oneof messages work in protobuf, its a bit tedious to make modifications to a Node object, in particular when changing the type of the inner object held within the Node in a generic code path (i.e. with a dynamic inner type), which required first knowing the name of the new inner object type, and then using public_send to set the new value. To help, introduce the new "inner" and "inner=" methods for PgQuery::Node, that get/set the inner object directly, avoiding the use of public_send. This will be of particular help for anyone utilizing the walk! API to make modifications to the query tree. To illustrate, an example is added to the treewalker spec showing how to utilize this.
This simplifies the common use case of just working with the walked node directly, which is now sufficient more often thanks to the addition of Node#inner= streamling modification of Node objects without going to the parent to replace them.
cc7d11c
to
3d7dcc4
Compare
Per request (in a different channel), I've run a quick benchmark showing that this doesn't impact the time the treewalker takes negatively, which it does not: Before (6 longest SQL statements in fingerprint test file):
After (6 longest SQL statements in fingerprint test file):
Benchmark script:# spec/benchmark.rb
#
# Needs "s.add_development_dependency 'benchmark-memory', '~> 0'" in the gemspec
require 'pg_query'
require 'json'
require 'benchmark'
require 'benchmark/memory'
def fingerprint_defs
# Sort input SQL by size (largest first), since those would be most interesting usually
@fingerprint_defs ||= JSON.parse(File.read(File.join(__dir__, 'files/fingerprint.json'))).sort_by { |d| -d['input'].size }
end
puts 'Benchmarking time for parse+walk...'
Benchmark.bmbm do |x|
fingerprint_defs.each do |testdef|
x.report(testdef['expectedHash']) { PgQuery.parse(testdef['input']).walk! { |_, _, _, _| } }
end
end
puts 'Benchmarking memory for parse+walk...'
Benchmark.memory do |x|
fingerprint_defs.each do |testdef|
x.report(testdef['expectedHash']) { PgQuery.parse(testdef['input']).walk! { |_, _, _, _| } }
end
end |
Because of how oneof messages work in protobuf, its a bit tedious to make modifications to a Node object, in particular when changing the type of the inner object held within the Node in a generic code path (i.e. with a dynamic inner type), which required first knowing the name of the new inner object type, and then using public_send to set the new value.
To help, introduce the new "inner" and "inner=" methods for PgQuery::Node, that get/set the inner object directly, avoiding the use of public_send.
This will be of particular help for anyone utilizing the walk! API to make modifications to the query tree. To illustrate, an example is added to the treewalker spec showing how to utilize this.
Additionally, this makes utilizing the
walk!
treewalker easier for the common use case of working with the node directly (without using the parent or location information).