-
Notifications
You must be signed in to change notification settings - Fork 9
feat: Update table names in metadata #994
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
WalkthroughThis change makes Changes
Sequence Diagram(s)sequenceDiagram
participant Planner
participant Extensions
participant MetaDataUtils
participant Metadata
Planner->>Extensions: Retrieve outputTable (null-safe)
Extensions-->>Planner: Return outputTable or fallback
Planner->>MetaDataUtils: Call layer(..., Some(outputTable))
MetaDataUtils->>Metadata: Copy metadata, set executionInfo.outputTable
MetaDataUtils-->>Planner: Return layered metadata
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/main/scala/ai/chronon/api/planner/MetaDataUtils.scala (1)
43-46: Comment-code mismatch on output table source.The comment states "use the base metadata's output table" but the code now uses
copy.outputTable. This appears inconsistent unless the copy and base metadata have identical output tables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/src/main/scala/ai/chronon/api/Extensions.scala(1 hunks)api/src/main/scala/ai/chronon/api/planner/GroupByPlanner.scala(3 hunks)api/src/main/scala/ai/chronon/api/planner/MetaDataUtils.scala(1 hunks)api/src/main/scala/ai/chronon/api/planner/MonolithJoinPlanner.scala(2 hunks)api/src/main/scala/ai/chronon/api/planner/StagingQueryPlanner.scala(2 hunks)
🧠 Learnings (5)
📓 Common learnings
Learnt from: tchow-zlai
PR: zipline-ai/chronon#263
File: cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala:56-57
Timestamp: 2025-01-24T23:55:40.650Z
Learning: For BigQuery table creation operations in BigQueryFormat.scala, allow exceptions to propagate directly without wrapping them in try-catch blocks, as the original BigQuery exceptions provide sufficient context.
api/src/main/scala/ai/chronon/api/planner/MonolithJoinPlanner.scala (2)
Learnt from: nikhil-zlai
PR: #70
File: service/src/main/java/ai/chronon/service/ApiProvider.java:6-6
Timestamp: 2024-12-03T04:04:33.809Z
Learning: The import scala.util.ScalaVersionSpecificCollectionsConverter in service/src/main/java/ai/chronon/service/ApiProvider.java is correct and should not be flagged in future reviews.
Learnt from: nikhil-zlai
PR: #50
File: spark/src/main/scala/ai/chronon/spark/stats/drift/SummaryUploader.scala:19-47
Timestamp: 2024-11-03T14:51:40.825Z
Learning: In Scala, the grouped method on collections returns an iterator, allowing for efficient batch processing without accumulating all records in memory.
api/src/main/scala/ai/chronon/api/planner/StagingQueryPlanner.scala (1)
Learnt from: nikhil-zlai
PR: #70
File: service/src/main/java/ai/chronon/service/ApiProvider.java:6-6
Timestamp: 2024-12-03T04:04:33.809Z
Learning: The import scala.util.ScalaVersionSpecificCollectionsConverter in service/src/main/java/ai/chronon/service/ApiProvider.java is correct and should not be flagged in future reviews.
api/src/main/scala/ai/chronon/api/planner/GroupByPlanner.scala (1)
Learnt from: nikhil-zlai
PR: #50
File: spark/src/main/scala/ai/chronon/spark/stats/drift/SummaryUploader.scala:19-47
Timestamp: 2024-11-03T14:51:40.825Z
Learning: In Scala, the grouped method on collections returns an iterator, allowing for efficient batch processing without accumulating all records in memory.
api/src/main/scala/ai/chronon/api/Extensions.scala (1)
Learnt from: tchow-zlai
PR: #263
File: cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala:56-57
Timestamp: 2025-01-24T23:55:40.650Z
Learning: For BigQuery table creation operations in BigQueryFormat.scala, allow exceptions to propagate directly without wrapping them in try-catch blocks, as the original BigQuery exceptions provide sufficient context.
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: tchow-zlai
PR: zipline-ai/chronon#263
File: cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala:56-57
Timestamp: 2025-01-24T23:55:40.650Z
Learning: For BigQuery table creation operations in BigQueryFormat.scala, allow exceptions to propagate directly without wrapping them in try-catch blocks, as the original BigQuery exceptions provide sufficient context.
api/src/main/scala/ai/chronon/api/planner/MonolithJoinPlanner.scala (2)
Learnt from: nikhil-zlai
PR: #70
File: service/src/main/java/ai/chronon/service/ApiProvider.java:6-6
Timestamp: 2024-12-03T04:04:33.809Z
Learning: The import scala.util.ScalaVersionSpecificCollectionsConverter in service/src/main/java/ai/chronon/service/ApiProvider.java is correct and should not be flagged in future reviews.
Learnt from: nikhil-zlai
PR: #50
File: spark/src/main/scala/ai/chronon/spark/stats/drift/SummaryUploader.scala:19-47
Timestamp: 2024-11-03T14:51:40.825Z
Learning: In Scala, the grouped method on collections returns an iterator, allowing for efficient batch processing without accumulating all records in memory.
api/src/main/scala/ai/chronon/api/planner/StagingQueryPlanner.scala (1)
Learnt from: nikhil-zlai
PR: #70
File: service/src/main/java/ai/chronon/service/ApiProvider.java:6-6
Timestamp: 2024-12-03T04:04:33.809Z
Learning: The import scala.util.ScalaVersionSpecificCollectionsConverter in service/src/main/java/ai/chronon/service/ApiProvider.java is correct and should not be flagged in future reviews.
api/src/main/scala/ai/chronon/api/planner/GroupByPlanner.scala (1)
Learnt from: nikhil-zlai
PR: #50
File: spark/src/main/scala/ai/chronon/spark/stats/drift/SummaryUploader.scala:19-47
Timestamp: 2024-11-03T14:51:40.825Z
Learning: In Scala, the grouped method on collections returns an iterator, allowing for efficient batch processing without accumulating all records in memory.
api/src/main/scala/ai/chronon/api/Extensions.scala (1)
Learnt from: tchow-zlai
PR: #263
File: cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigQueryFormat.scala:56-57
Timestamp: 2025-01-24T23:55:40.650Z
Learning: For BigQuery table creation operations in BigQueryFormat.scala, allow exceptions to propagate directly without wrapping them in try-catch blocks, as the original BigQuery exceptions provide sufficient context.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: online_tests
- GitHub Check: service_commons_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: service_tests
- GitHub Check: api_tests
- GitHub Check: flink_tests
- GitHub Check: aggregator_tests
- GitHub Check: join_tests
- GitHub Check: spark_tests
- GitHub Check: streaming_tests
- GitHub Check: analyzer_tests
- GitHub Check: fetcher_tests
- GitHub Check: batch_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (7)
api/src/main/scala/ai/chronon/api/planner/MonolithJoinPlanner.scala (2)
3-4: Import cleanup looks good.Wildcard import simplifies the imports while maintaining functionality.
35-40: Consistent output table propagation.The explicit passing of output table metadata to
MetaDataUtils.layeraligns with the coordinated updates across planners.api/src/main/scala/ai/chronon/api/planner/StagingQueryPlanner.scala (2)
4-4: Import addition supports enhanced metadata handling.Wildcard import enables access to the updated Extensions functionality.
25-26: Consistent parameter passing to metadata layer.Adding explicit output table parameter maintains consistency with other planner updates.
api/src/main/scala/ai/chronon/api/planner/GroupByPlanner.scala (3)
36-37: Explicit output table for backfill node.Correctly passes output table metadata for backfill operations.
83-83: Simplified parameter list for uploadToKV.Removing output table parameter suggests different metadata requirements for this node type.
108-108: Consistent with uploadToKV approach.Streaming node follows same pattern as uploadToKV for metadata layering.
bebc4e5 to
c1b1658
Compare
| val streamingOutputTableInfo = streamingNode.metaData.executionInfo.outputTableInfo | ||
| streamingOutputTableInfo should not be null | ||
| streamingOutputTableInfo.table should equal(gb.metaData.name + "__streaming") | ||
| streamingOutputTableInfo.table should equal(gb.metaData.outputNamespace + "." + gb.metaData.name + "__streaming") |
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.
| streamingOutputTableInfo.table should equal(gb.metaData.outputNamespace + "." + gb.metaData.name + "__streaming") | |
| streamingOutputTableInfo.table should equal(gb.metaData.outputTable + "__streaming") |
| val outputTableInfo = uploadToKVNode.metaData.executionInfo.outputTableInfo | ||
| outputTableInfo should not be null | ||
| outputTableInfo.table should equal(gb.metaData.name + "__uploadToKV") | ||
| outputTableInfo.table should equal(gb.metaData.outputNamespace + "." + gb.metaData.name + "__uploadToKV") |
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.
| outputTableInfo.table should equal(gb.metaData.outputNamespace + "." + gb.metaData.name + "__uploadToKV") | |
| outputTableInfo.table should equal(gb.metaData.outputTable + "__uploadToKV") |
2048539 to
a2d365b
Compare
Summary
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Tests