-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Implemented load function for different time columns #17085
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: UserDefinedTime
Are you sure you want to change the base?
Conversation
5c3959c to
0aa1d9c
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.
Pull request overview
This PR implements support for loading TsFiles with different time column configurations. When loading a TsFile into a table, the system now detects if the time column is at a different position or is present/absent compared to the existing table schema, and sets a flag to trigger appropriate decoding.
Changes:
- Added detection logic for time column position mismatches during table schema validation
- Propagated
needDecode4DifferentTimeColumnflag through the load pipeline - Updated method signatures to accept an AtomicBoolean parameter for tracking time column differences
- Added support for TIME column category in schema validation and creation
- Improved code quality with refactoring (PathUtils usage, final modifiers, comment corrections)
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TableHeaderSchemaValidator.java | Added logic to detect time column position differences and support TIME column creation |
| LoadTsFileTableSchemaCache.java | Added AtomicBoolean flag to track time column differences |
| LoadTsFileAnalyzer.java | Propagates needDecode4TimeColumn flag to LoadTsFile statement |
| Metadata.java | Updated interface with new parameter and improved javadoc |
| TableMetadataImpl.java | Updated implementation to pass through new parameter |
| LoadTsFile.java | Added needDecode4TimeColumn field and methods |
| RelationPlanner.java | Passes needDecode4TimeColumn flag to LoadTsFileNode |
| LoadTsFileNode.java | Added needDecode4TimeColumn parameter |
| LoadSingleTsFileNode.java | Added needDecodeTsFile parameter for early-return check |
| LogicalPlanVisitor.java | Updated LoadTsFileNode construction with new parameter |
| TsTable.java | Made columnSchemaMap final (code quality improvement) |
| LoadTsFileManager.java | Refactored to use PathUtils.isTableModelDatabase() |
| Test files | Updated mocks to use new method signature |
| IoTDBLoadTsFileIT.java | Added comprehensive test for time column scenarios |
| TsFileTableGenerator.java | Added filtering logic for TIME columns (with bug) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final List<IMeasurementSchema> schemaWithoutTime = | ||
| schemas.stream() | ||
| .filter(schema -> !schema.getMeasurementName().equals("time")) | ||
| .collect(Collectors.toList()); |
Copilot
AI
Jan 27, 2026
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.
The filtering logic for TIME columns is inconsistent. The schemas are filtered by checking if the measurement name equals "time" (line 86), but the column categories are filtered by checking if the category is TIME (line 102). If a TIME column has a different name (e.g., "time1"), it won't be filtered from schemaWithoutTime but will be filtered from the column categories, causing a size mismatch between the lists passed to the Tablet constructor. Consider filtering schemas by checking the corresponding category from columnCategoryList instead of hardcoding the name check.
Description
As the title said.
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR