-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Adding support for big decimal aggregations via MSQ #18164
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
Conversation
* Minor refactoring to move classes into separate packages.
SELECT | ||
TIME_PARSE(TRIM("timestamp")) AS "__time", | ||
"itemName", | ||
BIG_SUM("saleAmount") as amount |
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.
I guess descendants of CompressedBigDecimalSqlAggregatorTestBase
is not run under MSQ as it would be weird to load that under the MSQ module.
I wonder if it would be possible to either run those tests somehow from the quidem-ut
module - or possibly just exercise some query via an iq
file from the same module to ensure that it can't be broken too easily
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.
To me it would make sense to test this by either:
- loading MSQ into the compressed-big-decimal extension as a test dependency;
- or, loading both in
quidem-ut
.
SELECT | ||
TIME_PARSE(TRIM("timestamp")) AS "__time", | ||
"itemName", | ||
BIG_SUM("saleAmount") as amount |
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.
To me it would make sense to test this by either:
- loading MSQ into the compressed-big-decimal extension as a test dependency;
- or, loading both in
quidem-ut
.
@Override | ||
public AggregatorFactory withName(String newName) | ||
{ | ||
return new CompressedBigDecimalMaxAggregatorFactory(newName, fieldName, size, scale, strictNumberParsing); |
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.
Was this the fix (along with similar changes in min, sum)?
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.
Yes it was adding the withName implementation to the other aggregator factories.
@@ -144,6 +144,31 @@ IngestionSpec syntax: | |||
} | |||
} | |||
``` | |||
|
|||
Sql based ingestion sample query: |
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.
"SQL-based" is how we usually write this.
* Review comments
PTAL. |
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.
thank you @cryptoe for the updates!
I think you will need to add the |
Release note
This PR has: