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

remove size from default analysisTypes list for segmentMetadata query #3773

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

himanshug
Copy link
Contributor

many external tools (e.g. pivot, caravel etc) use SegmentMetadata query to introspect columns in a dataSource and they do not need the column size. size is slower to compute for string columns and slows down whole query, most people incur that additional cost just because it is included there by default.

Technically, it is backward incompatible so I am willing to move its milestone to 0.10.0 as well.

@himanshug himanshug added this to the 0.9.3 milestone Dec 12, 2016
@fjy
Copy link
Contributor

fjy commented Dec 12, 2016

I'm really in favor of this as the size is basically non-sensical

@b-slim
Copy link
Contributor

b-slim commented Dec 13, 2016

👍

@b-slim
Copy link
Contributor

b-slim commented Dec 13, 2016

@himanshug can you please fix the UTs plus integration testing as well.

SegmentMetadataQueryTest.testSegmentMetadataQueryWithDefaultAnalysisMerge:481->testSegmentMetadataQueryWithDefaultAnalysisMerge:572 failed SegmentMetadata merging query-0 expected:<SegmentAnalysis{id='testSegment', interval=[2011-01-12T00:00:00.000Z/2011-04-15T00:00:00.001Z], columns={__time=ColumnAnalysis{type='LONG', hasMultipleValues=false, size=24180, cardinality=null, minValue=null, maxValue=null, errorMessage='null'}, index=ColumnAnalysis{type='FLOAT', hasMultipleValues=false, size=19344, cardinality=null, minValue=null, maxValue=null, errorMessage='null'}, placement=ColumnAnalysis{type='STRING', hasMultipleValues=false, size=21645, cardinality=1, minValue=preferred, maxValue=preferred, errorMessage='null'}}, size=188261, numRows=2418, aggregators=null, timestampSpec=null, queryGranularity=null, rollup=null}> but was:<SegmentAnalysis{id='testSegment', interval=[2011-01-12T00:00:00.000Z/2011-04-15T00:00:00.001Z], columns={__time=ColumnAnalysis{type='LONG', hasMultipleValues=false, size=0, cardinality=null, minValue=null, maxValue=null, errorMessage='null'}, index=ColumnAnalysis{type='FLOAT', hasMultipleValues=false, size=0, cardinality=null, minValue=null, maxValue=null, errorMessage='null'}, placement=ColumnAnalysis{type='STRING', hasMultipleValues=false, size=0, cardinality=1, minValue=preferred, maxValue=preferred, errorMessage='null'}}, size=0, numRows=2418, aggregators=null, timestampSpec=null, queryGranularity=null, rollup=null}>

@himanshug himanshug force-pushed the seg_metadata_size_removal branch 2 times, most recently from 535d5c6 to 31964da Compare December 13, 2016 17:40
@fjy
Copy link
Contributor

fjy commented Dec 13, 2016

👍 after fixing integration tests

@himanshug himanshug force-pushed the seg_metadata_size_removal branch from 31964da to cdcf521 Compare December 13, 2016 21:26
@fjy fjy closed this Dec 14, 2016
@fjy fjy reopened this Dec 14, 2016
@b-slim b-slim merged commit ed322a4 into apache:master Dec 14, 2016
@himanshug himanshug deleted the seg_metadata_size_removal branch January 3, 2017 16:24
@clambertus clambertus unassigned fjy and b-slim Jul 6, 2018
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.

None yet

3 participants