-
Notifications
You must be signed in to change notification settings - Fork 90
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 27 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
2714ad5
d70cfa7
053fd9d
99f9788
7dff16e
5852a1b
c29a1df
bb9b7d2
2f78a05
b296d70
3202e7d
fa2fcba
b0e90bf
7138a66
325302c
40aa755
b847a40
7692f24
b385cda
464395b
1002744
cd141ab
9a25d10
a9f3c78
0a8499d
91d2768
5649e0c
c68ae66
eea3bbf
127410c
0dc3879
84d3ece
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 |
|---|---|---|
|
|
@@ -98,7 +98,7 @@ object Extensions { | |
| def cleanName: String = metaData.name.sanitize | ||
|
|
||
| def outputTable = s"${metaData.outputNamespace}.${metaData.cleanName}" | ||
|
|
||
| def incrementalOutputTable = s"${metaData.outputNamespace}.${metaData.cleanName}_inc" | ||
kambstreat marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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" | ||
|
|
@@ -179,8 +179,13 @@ object Extensions { | |
|
|
||
| def outputColumnName = | ||
| s"${aggregationPart.inputColumn}_$opSuffix${aggregationPart.window.suffix}${bucketSuffix}" | ||
|
|
||
| def incrementalOutputColumnName = | ||
| s"${aggregationPart.inputColumn}_$opSuffix${bucketSuffix}" | ||
|
Collaborator
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
Author
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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -466,7 +466,8 @@ object Driver { | |
| tableUtils, | ||
| args.stepDays.toOption, | ||
| args.startPartitionOverride.toOption, | ||
| !args.runFirstHole() | ||
| !args.runFirstHole(), | ||
| args.groupByConf.isIncremental | ||
|
||
| ) | ||
|
|
||
| if (args.shouldExport()) { | ||
|
|
||
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.