-
Notifications
You must be signed in to change notification settings - Fork 83
Write daily IRs for incremental aggregation #998
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
base: main
Are you sure you want to change the base?
Changes from all commits
eaacfa6
40b6cb2
a014b6e
32d559e
37293df
6263706
cb4325b
796ef96
f218b23
b1d4ee9
2ab7659
238c781
8edfd27
e903683
0bdc4fc
7987931
2b26d45
7b62a43
aeeb5ec
9180d23
ee81672
29a3f28
aa16010
ff41cc9
aa25f9f
3efe8cd
a3bece6
897d18c
a52d7d0
9c446ec
ca14309
dfb9226
7ca4dfc
ab994bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -97,7 +97,7 @@ object Extensions { | |||||
| def cleanName: String = metaData.name.sanitize | ||||||
|
|
||||||
| def outputTable = s"${metaData.outputNamespace}.${metaData.cleanName}" | ||||||
|
|
||||||
| def incrementalOutputTable = s"${metaData.outputNamespace}.${metaData.cleanName}_inc" | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @abbywh I think we already have a daily here. |
||||||
| def preModelTransformsTable = s"${metaData.outputNamespace}.${metaData.cleanName}_pre_mt" | ||||||
| def outputLabelTable = s"${metaData.outputNamespace}.${metaData.cleanName}_labels" | ||||||
| def outputFinalView = s"${metaData.outputNamespace}.${metaData.cleanName}_labeled" | ||||||
|
|
@@ -178,8 +178,13 @@ object Extensions { | |||||
|
|
||||||
| def outputColumnName = | ||||||
| s"${aggregationPart.inputColumn}_$opSuffix${aggregationPart.window.suffix}${bucketSuffix}" | ||||||
|
|
||||||
| def incrementalOutputColumnName = | ||||||
| s"${aggregationPart.inputColumn}_$opSuffix${bucketSuffix}" | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we still keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pengyu-hou not sure I get it. I can not use the window.suffix right as the intermediate incremental is daily aggregation. |
||||||
|
|
||||||
| } | ||||||
|
|
||||||
|
|
||||||
| implicit class AggregationOps(aggregation: Aggregation) { | ||||||
|
|
||||||
| // one agg part per bucket per window | ||||||
|
|
||||||
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.
It looks to me like this mostly exists to serialize aggregators. Is there something more generic we can do? @nikhilsimha and I talked about https://fory.apache.org/ for example, but it could be even simpler to moving this logic into a different class abstraction
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 agree that if we could somehow make it more general, then we would not have to implement for each operation.
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.
also cc @nikhil-zlai 😁
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 was thinking of sticking with Avro because it is what we used for the online serving path so that we can keep the ser//de logic same.
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 think the issue with Avro is that it needs to schematize the object, whereas hopIR is just an Array[Any]. If you just reflect the hopIR, there is no need to schematize it.