-
Notifications
You must be signed in to change notification settings - Fork 0
[JAY-625] Add Elasticsearch's composite aggregation #32
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
Changes from all commits
1725960
b6a2f24
851233e
dee3feb
e187774
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'active_support' | ||
| require 'active_support/core_ext/string/inflections' | ||
|
|
||
| require_relative 'aggregation' | ||
| require_relative 'sources/sources' | ||
| require_relative 'errors/aggregations_error' | ||
|
|
||
| module JayAPI | ||
| module Elasticsearch | ||
| class QueryBuilder | ||
| class Aggregations | ||
| # Represents a Composite aggregation in Elasticsearch. For more | ||
| # information about this type of aggregation: | ||
| # @see https://www.elastic.co/docs/reference/aggregations/search-aggregations-bucket-composite-aggregation | ||
| class Composite < ::JayAPI::Elasticsearch::QueryBuilder::Aggregations::Aggregation | ||
| attr_reader :size | ||
|
|
||
| # @param [String] name The name of the composite aggregation. | ||
| # @param [Integer] size The number of composite buckets to return. | ||
| # @yieldparam [JayAPI::Elasticsearch::QueryBuilder::Aggregations::Sources::Sources] | ||
| # The collection of sources for the composite aggregation. This | ||
| # should be used by the caller to add sources to the composite | ||
| # aggregation. | ||
| # @raise [JayAPI::Elasticsearch::QueryBuilder::Aggregations::Errors::AggregationsError] | ||
| # If the method is called without a block. | ||
| def initialize(name, size: nil, &block) | ||
| unless block | ||
| raise(::JayAPI::Elasticsearch::QueryBuilder::Aggregations::Errors::AggregationsError, | ||
| "The #{self.class.name.demodulize} aggregation must be initialized with a block") | ||
| end | ||
|
|
||
| super(name) | ||
| @size = size | ||
| block.call(sources) | ||
| end | ||
|
|
||
| # @return [self] A copy of the receiver. Sources and nested | ||
| # aggregations are also cloned. | ||
| def clone | ||
| # rubocop:disable Lint/EmptyBlock (The sources will be assigned later) | ||
| copy = self.class.new(name, size: size) {} | ||
| # rubocop:enable Lint/EmptyBlock | ||
|
|
||
| copy.aggregations = aggregations.clone | ||
| copy.sources = sources.clone | ||
| copy | ||
| end | ||
|
|
||
| # @return [Hash] The Hash representation of the +Aggregation+. | ||
| # Properly formatted for Elasticsearch. | ||
| def to_h | ||
| super do | ||
| { | ||
| composite: { | ||
| sources: sources.to_a, | ||
| size: size | ||
| }.compact | ||
| } | ||
| end | ||
| end | ||
|
|
||
| protected | ||
|
|
||
| attr_writer :sources # Used by the #clone method | ||
|
|
||
| # @return [JayAPI::Elasticsearch::QueryBuilder::Aggregations::Sources::Sources] | ||
| # The collection of sources of the composite aggregation. | ||
| def sources | ||
| @sources ||= ::JayAPI::Elasticsearch::QueryBuilder::Aggregations::Sources::Sources.new | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require_relative 'terms' | ||
|
|
||
| module JayAPI | ||
| module Elasticsearch | ||
| class QueryBuilder | ||
| class Aggregations | ||
| module Sources | ||
| # Represents the collection of sources for a Composite aggregation in | ||
| # Elasticsearch | ||
| class Sources | ||
|
Comment on lines
+9
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first I had the class also as namespace, so instead of So this is why at the end I decided to just add a namespace module. I have no issue with changing the name, but.... I couldn't think of a better option, do you have a suggestion? |
||
| # Adds a +terms+ source to the collection. | ||
| # For information about the parameters: | ||
| # @see Sources::Terms#initialize | ||
| def terms(name, **kw_args) | ||
| sources.push(::JayAPI::Elasticsearch::QueryBuilder::Aggregations::Sources::Terms.new(name, **kw_args)) | ||
| end | ||
|
|
||
| # @return [Array<Hash>] Array representation of the collection of | ||
| # sources of the composite aggregation. | ||
| def to_a | ||
| sources.map(&:to_h) | ||
| end | ||
|
|
||
| # @return [self] A copy of the receiver (not a shallow clone, it | ||
| # clones all of the elements of the collection). | ||
| def clone | ||
| self.class.new.tap do |copy| | ||
| copy.sources.concat(sources.map(&:clone)) | ||
| end | ||
| end | ||
|
|
||
| protected | ||
|
|
||
| # @return [Array<Object>] The array used to hold the collection of | ||
| # sources. | ||
| def sources | ||
| @sources ||= [] | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module JayAPI | ||
| module Elasticsearch | ||
| class QueryBuilder | ||
| class Aggregations | ||
| module Sources | ||
| # Represents a "Terms" value source for a Composite aggregation. | ||
| # More information about this type of value source can be found here: | ||
| # https://www.elastic.co/docs/reference/aggregations/search-aggregations-bucket-composite-aggregation#_terms | ||
| class Terms | ||
| attr_reader :name, :field, :order, :missing_bucket, :missing_order | ||
|
|
||
| # @param [String] name The name for the value source. | ||
| # @param [String] field The field for the value source. | ||
| # @param [String, nil] order The order in which the values coming | ||
| # from this data source should be ordered, this can be either | ||
| # "asc" or "desc" | ||
| # @param [Boolean] missing_bucket Whether or not a bucket for the | ||
| # documents without a value in +field+ should be created. | ||
| # @param [String] missing_order Where to put the bucket for the | ||
| # documents with a missing value, either "first" or "last". | ||
| def initialize(name, field:, order: nil, missing_bucket: nil, missing_order: nil) | ||
| @name = name | ||
| @field = field | ||
| @order = order | ||
| @missing_bucket = missing_bucket | ||
| @missing_order = missing_order | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No check on this string (same thing for
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not usually doing any checking in these classes. If you pass Let me know if you disagree. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you think the Elasticsearch error is explicative enough then sure 👍 |
||
| end | ||
|
|
||
| # @return [self] A copy of the receiver. | ||
| def clone | ||
| self.class.new( | ||
| name, field: field, order: order, missing_bucket: missing_bucket, missing_order: missing_order | ||
| ) | ||
| end | ||
|
|
||
| # @return [Hash] The hash representation for the value source. | ||
| def to_h | ||
| { | ||
| name => { | ||
| terms: { | ||
| field: field, | ||
| order: order, | ||
| missing_bucket: missing_bucket, | ||
| missing_order: missing_order | ||
| }.compact | ||
| } | ||
| } | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'jay_api/elasticsearch/query_builder/aggregations/composite' | ||
|
|
||
| RSpec.describe JayAPI::Elasticsearch::QueryBuilder::Aggregations::Composite do | ||
| subject(:composite) do | ||
| described_class.new('products_by_brand', **constructor_params) do |sources| | ||
| sources.terms('product', field: 'product.name', order: 'asc') | ||
| sources.terms('brand', field: 'brand.name') | ||
| end | ||
| end | ||
|
|
||
| let(:constructor_params) { {} } | ||
|
|
||
| describe '#to_h' do | ||
| subject(:method_call) { composite.to_h } | ||
|
|
||
| let(:expected_hash) do | ||
| { | ||
| 'products_by_brand' => { | ||
| composite: { | ||
| sources: [ | ||
| { 'product' => { terms: { field: 'product.name', order: 'asc' } } }, | ||
| { 'brand' => { terms: { field: 'brand.name' } } } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| end | ||
|
|
||
| it 'returns the expected Hash' do | ||
| expect(method_call).to eq(expected_hash) | ||
| end | ||
|
|
||
| context "when a 'size' has been specified" do | ||
| let(:constructor_params) { { size: 10 } } | ||
|
|
||
| let(:expected_hash) do | ||
| { | ||
| 'products_by_brand' => { | ||
| composite: { | ||
| sources: [ | ||
| { 'product' => { terms: { field: 'product.name', order: 'asc' } } }, | ||
| { 'brand' => { terms: { field: 'brand.name' } } } | ||
| ], | ||
| size: 10 | ||
| } | ||
| } | ||
| } | ||
| end | ||
|
|
||
| it 'returns the expected Hash' do | ||
| expect(method_call).to eq(expected_hash) | ||
| end | ||
| end | ||
|
|
||
| context 'with nested aggregations' do | ||
| before do | ||
| composite.aggs do |aggs| | ||
| aggs.avg('avg_price', field: 'product.price') | ||
| end | ||
| end | ||
|
|
||
| let(:expected_hash) do | ||
| { | ||
| 'products_by_brand' => { | ||
| composite: { | ||
| sources: [ | ||
| { 'product' => { terms: { field: 'product.name', order: 'asc' } } }, | ||
| { 'brand' => { terms: { field: 'brand.name' } } } | ||
| ] | ||
| }, | ||
| aggs: { | ||
| 'avg_price' => { avg: { field: 'product.price' } } | ||
| } | ||
| } | ||
| } | ||
| end | ||
|
|
||
| it 'returns the expected Hash' do | ||
| expect(method_call).to eq(expected_hash) | ||
| end | ||
| end | ||
| end | ||
| end |
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.
Similar question for the classes introduced in the 2 previous commits:
I'm seeing that these
#clone,#to_h,#to_amethods are not shown to be used in the docs you add in the comings commits. Are these methods then only used to test these classes? No problem for me, but this makes it so that around 50% of the logic in the class is used only to test the class itself, which is a bit strange IMO (although as I said, no problem)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.
These methods are normally not used directly, hence they are not listed in the documentation.
#to_hand#to_aare used when you call#to_queryon theQueryBuilderinstance, there, all these classes are converted into a Hash that is then sent to Elasticsearch. So, basically these are serialization methods.#to_hand#to_amethods are called recursively until everything is either a hash or an array.Regarding
#clonethey are used in two casesQueryBuilderobject, then all the internal classes are cloned recursively, this prevents changes to clone from affecting the originalQueryBuilderobject.QueryBuilderobjects together. The resulting merged object also gets a clone of all the underlying classes. Again, this prevents changes to the merged object from affecting the original queries.