replace partition spec with column #776
Conversation
WalkthroughThe changes remove the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Planner
participant PartitionSpec
Caller->>Planner: new Planner(...)(implicit partitionSpec: PartitionSpec)
Planner->>PartitionSpec: Access partition fields directly
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (31)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/main/scala/ai/chronon/api/planner/PlanNode.scala (1)
46-46: Improved parameter namingParameter renamed from
constoconfsfor better clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (13)
api/src/main/scala/ai/chronon/api/planner/GroupByOfflinePlanner.scala(1 hunks)api/src/main/scala/ai/chronon/api/planner/JoinOfflinePlanner.scala(1 hunks)api/src/main/scala/ai/chronon/api/planner/MetaDataUtils.scala(3 hunks)api/src/main/scala/ai/chronon/api/planner/NodeRunner.scala(1 hunks)api/src/main/scala/ai/chronon/api/planner/PartitionSpecWithColumn.scala(0 hunks)api/src/main/scala/ai/chronon/api/planner/PlanNode.scala(1 hunks)api/src/main/scala/ai/chronon/api/planner/Planner.scala(1 hunks)api/src/main/scala/ai/chronon/api/planner/TableDependencies.scala(2 hunks)api/thrift/orchestration.thrift(1 hunks)spark/src/main/scala/ai/chronon/spark/Driver.scala(1 hunks)spark/src/main/scala/ai/chronon/spark/JoinUtils.scala(1 hunks)spark/src/main/scala/ai/chronon/spark/batch/MergeJob.scala(1 hunks)spark/src/main/scala/ai/chronon/spark/catalog/TableUtils.scala(0 hunks)
💤 Files with no reviewable changes (2)
- api/src/main/scala/ai/chronon/api/planner/PartitionSpecWithColumn.scala
- spark/src/main/scala/ai/chronon/spark/catalog/TableUtils.scala
🧰 Additional context used
🧬 Code Graph Analysis (1)
spark/src/main/scala/ai/chronon/spark/Driver.scala (1)
api/src/main/scala/ai/chronon/api/planner/RelevantLeftForJoinPart.scala (2)
RelevantLeftForJoinPart(16-24)RelevantLeftForJoinPart(26-92)
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: service_commons_tests
- GitHub Check: service_tests
- GitHub Check: streaming_tests
- GitHub Check: service_tests
- GitHub Check: streaming_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: join_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: analyzer_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: groupby_tests
- GitHub Check: online_tests
- GitHub Check: spark_tests
- GitHub Check: online_tests
- GitHub Check: fetcher_tests
- GitHub Check: groupby_tests
- GitHub Check: flink_tests
- GitHub Check: flink_tests
- GitHub Check: analyzer_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: batch_tests
- GitHub Check: api_tests
- GitHub Check: spark_tests
- GitHub Check: api_tests
- GitHub Check: join_tests
- GitHub Check: aggregator_tests
- GitHub Check: batch_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: aggregator_tests
- GitHub Check: fetcher_tests
🔇 Additional comments (18)
spark/src/main/scala/ai/chronon/spark/Driver.scala (1)
22-22: Simplified import statementImport statement updated to remove
PartitionSpecWithColumn, aligning with broader refactoring to replace it withPartitionSpecthroughout the codebase.spark/src/main/scala/ai/chronon/spark/batch/MergeJob.scala (1)
5-5: Simplified import statementImport statement updated to remove
PartitionSpecWithColumnreference, consistent with codebase-wide refactoring.api/thrift/orchestration.thrift (1)
265-265: Added LogicalType field to Conf structNew optional field
logicalTypeadded to classify configuration types.api/src/main/scala/ai/chronon/api/planner/Planner.scala (2)
3-3: Import updated to use PartitionSpec instead of PartitionSpecWithColumn.Correctly replaces the import with the new simplified type.
7-7: Parameter type updated from PartitionSpecWithColumn to PartitionSpec.Parameter type in the abstract Planner class correctly updated to match the codebase-wide refactoring.
api/src/main/scala/ai/chronon/api/planner/GroupByOfflinePlanner.scala (2)
3-3: Updated imports to use PartitionSpec.Import statement correctly updated to include PartitionSpec.
8-8: Updated constructor parameter type to PartitionSpec.Parameter type correctly changed to PartitionSpec to align with the refactoring.
api/src/main/scala/ai/chronon/api/planner/NodeRunner.scala (2)
3-3: Updated imports to include PartitionSpec.Import statement correctly updated.
7-7: Renamed method and changed return type to PartitionSpec.Both the method name and return type properly updated as part of the refactoring.
api/src/main/scala/ai/chronon/api/planner/JoinOfflinePlanner.scala (1)
14-14: Updated parameter type to PartitionSpec.Constructor parameter type correctly changed to match the codebase-wide refactoring.
spark/src/main/scala/ai/chronon/spark/JoinUtils.scala (2)
535-535: Updated to use partitionSpec instead of outputPartitionSpec.This change aligns with removing PartitionSpecWithColumn throughout the codebase.
539-539: Updated to use partitionSpec instead of outputPartitionSpec.Consistent with the change in line 535 and aligns with the broader refactoring effort.
api/src/main/scala/ai/chronon/api/planner/TableDependencies.scala (3)
5-5: Updated imports to include PartitionSpec.Necessary change to support the new type structure.
9-9: Changed implicit parameter type from PartitionSpecWithColumn to PartitionSpec.Part of the refactoring to simplify partition specification handling.
63-65: Replaced dynamic partition column with hardcoded "dt" and simplified access to format and interval.The partition column is now hardcoded as "dt" instead of being derived from a wrapper type, consistent with refactoring approach.
api/src/main/scala/ai/chronon/api/planner/MetaDataUtils.scala (3)
4-13: Updated imports for cleaner organization.Expanded imports to include specific entities needed after refactoring.
34-34: Changed implicit parameter type from PartitionSpecWithColumn to PartitionSpec.Consistent with other changes in the codebase to simplify partition specification.
59-61: Simplified partition specification handling.Hardcoded partition column to "dt" and directly accessing fields from partitionSpec, consistent with the overall refactoring approach.
nikhil-zlai
left a comment
nikhil-zlai
left a comment
There was a problem hiding this comment.
why not use the partition column instead of hardcoded dt everywhere?
| .setPartitionColumn(partitionSpecWithColumn.partitionColumn) | ||
| .setPartitionFormat(partitionSpecWithColumn.partitionSpec.format) | ||
| .setPartitionInterval(WindowUtils.hours(partitionSpecWithColumn.partitionSpec.spanMillis)) | ||
| .setPartitionColumn("dt") |
varant-zlai
force-pushed
the
vz/replace_partition_spec_with_column
branch
from
de54d58 to
f0308e9
Compare
| .setPartitionColumn(specWithColumn.partitionColumn) | ||
| .setPartitionFormat(specWithColumn.partitionSpec.format) | ||
| .setPartitionInterval(WindowUtils.hours(specWithColumn.partitionSpec.spanMillis)) | ||
| .setPartitionColumn("dt") |
## Summary replace partition spec with column -> partiton spec ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Simplified partition specification handling across planners and utilities by removing the custom partition spec wrapper and standardizing on a single partition spec type. - Updated related methods and class constructors to use the new partition spec approach, streamlining partition metadata access. - Removed unused fields and imports related to the old partition spec wrapper. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: ezvz <[email protected]>
## Summary replace partition spec with column -> partiton spec ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Simplified partition specification handling across planners and utilities by removing the custom partition spec wrapper and standardizing on a single partition spec type. - Updated related methods and class constructors to use the new partition spec approach, streamlining partition metadata access. - Removed unused fields and imports related to the old partition spec wrapper. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: ezvz <[email protected]>
Summary
replace partition spec with column -> partiton spec
Checklist
Summary by CodeRabbit