-
-
Notifications
You must be signed in to change notification settings - Fork 657
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
add Key::I64 and Key::U64 variants in aggregation #2468
Conversation
Currently all `Key` numerical values are returned as f64. This causes problems in some cases with the precision and the way f64 is serialized. This PR adds `Key::I64` and `Key::U64` variants and uses them in the term aggregation.
2d3b27f
to
5def996
Compare
@@ -339,9 +341,18 @@ fn get_missing_val( | |||
Key::Str(_) if column_type == ColumnType::Str => Some(u64::MAX), | |||
// Allow fallback to number on text fields | |||
Key::F64(_) if column_type == ColumnType::Str => Some(u64::MAX), | |||
Key::U64(_) if column_type == ColumnType::Str => Some(u64::MAX), |
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 don't understand what get_missing_val
is about.
It is clearly doing something strange. Do you remember?
Can you comment/rename to make it make sense?
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, there is definitely a comment missing to explain it
/// Get the missing value as internal u64 representation
///
/// For terms we use u64::MAX as sentinel value
/// For numerical data we convert the value into the representation
/// we would get from the fast field, when we open it as u64_lenient_for_type.
///
/// That way we can use it the same way as if it would come from the fastfield.
@@ -191,6 +191,12 @@ impl SegmentCardinalityCollector { | |||
let val = f64_to_u64(*val); | |||
self.cardinality.sketch.insert_any(&val); | |||
} | |||
Key::U64(val) => { |
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.
sorry for using this PR..
.expect("Found placeholder term_ord but
missing is None");
I think we are just supposed to ignore the value.
8e6df52
to
fff15c5
Compare
* add Key::I64 and Key::U64 variants in aggregation Currently all `Key` numerical values are returned as f64. This causes problems in some cases with the precision and the way f64 is serialized. This PR adds `Key::I64` and `Key::U64` variants and uses them in the term aggregation. * add clarification comment
Currently all
Key
numerical values are returned as f64. This causes problems in somecases with the precision and the way f64 is serialized.
This PR adds
Key::I64
andKey::U64
variants and uses them in the termaggregation.
Fixes quickwit-oss/quickwit#5254