Skip to content

complex metrics -> complex types#12378

Closed
clintropolis wants to merge 4 commits intoapache:masterfrom
clintropolis:complex-type-coercion
Closed

complex metrics -> complex types#12378
clintropolis wants to merge 4 commits intoapache:masterfrom
clintropolis:complex-type-coercion

Conversation

@clintropolis
Copy link
Member

@clintropolis clintropolis commented Mar 29, 2022

Description

This PR rebrands ComplexMetricSerde as ComplexTypeSerde, ComplexMetrics as ComplexTypes, and ComplexMetricExtractor as ComplexTypeExtractor. All of the former are marked as deprecated, and their logic moved to (and implement) the latter to minimize disruption. All current serdes still implement ComplexMetricSerde rather than the new type, to allow anything using ComplexMetrics.getSerdeForType to continue functioning until their future removal, at which point the implementations shall be moved to extend ComplexTypeSerde directly.

Additionally, it adds a method to ComplexTypeExtractor, coerceValue, to use to allow COMPLEX types to coerce values to the appropriate type when read from a column value selector from RowBasedColumnSelectorFactory. This allows COMPLEX typed dimensions to perform value coercion, similar to the other use of the extractValue method when reading COMPLEX metrics for queries against IncrementalIndex.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

changes:
* rebrand ComplexMetricSerde as ComplexTypeSerde, ComplexMetrics as ComplexTypes, ComplexMetricExtractor as ComplexTypeExtractor
* add 'coerceValue' method to ComplexTypeExtractor to use to allow COMPLEX types to coerce values to the appropriate type when read from a column value selector from RowBasedColumnSelectorFactory
@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2022

This pull request introduces 1 alert when merging 1564c67 into f1841c6 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@clintropolis
Copy link
Member Author

btw, I tested that this change is minimally disruptive by doing some tests using extensions (datasketches specifically, for my complex types) built against the master branch without the changes in this PR, and everything was still functioning correctly as far as I can tell, so I think it should be relatively chill despite changing stuff marked @ExtensionPoint.

@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone Mar 30, 2022
@clintropolis
Copy link
Member Author

closing in favor of #12382, since need to think a bit more about how to do type coercion for complex dimensions (which cannot lean on agg deserialize or extractor methods to ensure that complex types are their intermediary working java type)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants