-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: implement earliest_by_offset() UDAF #5273
Conversation
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.
LGTM, with one request.
07cecf7
to
cf2d846
Compare
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.
LGTM. I think some constants and method could be reused (put in utility or base class) to avoid duplication.
static final String SEQ_FIELD = "SEQ"; | ||
static final String VAL_FIELD = "VAL"; | ||
|
||
public static final Schema STRUCT_INTEGER = SchemaBuilder.struct().optional() |
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.
All these constants look the same as the ones in LatestbyOffset. It would be nice to move them to another shared file instead of duplicating them.
return earliest(STRUCT_STRING); | ||
} | ||
|
||
static <T> Struct createStruct(final Schema schema, final T 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.
This could go in a common base class instead of duplicating (or a utils class)
return sequence.getAndIncrement(); | ||
} | ||
|
||
private static int compareStructs(final Struct struct1, final Struct struct2) { |
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.
This could go in common class too.
* specific language governing permissions and limitations under the License. | ||
*/ | ||
|
||
package io.confluent.ksql.function.udaf; |
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.
Imho we should create a directory and put both udafs and this file in the same package. That way it can be package protected.
Description
Fixes #5268
Implements earliest_by_offset() UDAF which computes the earliest value for a column. Earliest being defined as offset order.
Note for the reviewer:
Testing done
Added new unit test and QTT test
Reviewer checklist