Copilot commented on code in PR #9609:
URL: https://github.com/apache/incubator-gluten/pull/9609#discussion_r2085986800
##########
cpp-ch/local-engine/Parser/RelParsers/MergeTreeRelParser.cpp:
##########
@@ -83,6 +85,41 @@ void replaceFileNameNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInsta
actions_dag.addOrReplaceInOutputs(alias);
}
+void replaceFileSizeNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context)
+{
+ const auto int64_type = std::make_shared<DB::DataTypeInt64>();
+ actions_dag.addOrReplaceInOutputs(actions_dag.addColumn(
+ DB::ColumnWithTypeAndName(int64_type->createColumnConst(1, -1),
int64_type, FileMetaColumns::FILE_SIZE)));
Review Comment:
Using the magic number `-1` as a sentinel for file size may be unclear;
extract it into a named constant like `UNKNOWN_FILE_SIZE`.
```suggestion
DB::ColumnWithTypeAndName(int64_type->createColumnConst(1,
FileMetaColumns::UNKNOWN_FILE_SIZE), int64_type, FileMetaColumns::FILE_SIZE)));
```
##########
cpp-ch/local-engine/Parser/RelParsers/MergeTreeRelParser.cpp:
##########
@@ -83,6 +85,41 @@ void replaceFileNameNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInsta
actions_dag.addOrReplaceInOutputs(alias);
}
+void replaceFileSizeNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context)
+{
+ const auto int64_type = std::make_shared<DB::DataTypeInt64>();
+ actions_dag.addOrReplaceInOutputs(actions_dag.addColumn(
+ DB::ColumnWithTypeAndName(int64_type->createColumnConst(1, -1),
int64_type, FileMetaColumns::FILE_SIZE)));
+}
+
+void replaceFileModificationTimeNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context)
+{
+ const auto decimal64_type = std::make_shared<DB::DataTypeDateTime64>(6);
+ actions_dag.addOrReplaceInOutputs(actions_dag.addColumn(
+ DB::ColumnWithTypeAndName(decimal64_type->createColumnConst(1,
DecimalField<DateTime64>(0, 6)), decimal64_type,
FileMetaColumns::FILE_MODIFICATION_TIME)));
Review Comment:
The literal `0` in `DecimalField<DateTime64>(0, 6)` is a sentinel value;
consider defining a constant or adding a comment to explain its meaning.
```suggestion
constexpr DateTime64 DEFAULT_DATETIME64_VALUE = 0; // Default value for
DateTime64
actions_dag.addOrReplaceInOutputs(actions_dag.addColumn(
DB::ColumnWithTypeAndName(decimal64_type->createColumnConst(1,
DecimalField<DateTime64>(DEFAULT_DATETIME64_VALUE, 6)), decimal64_type,
FileMetaColumns::FILE_MODIFICATION_TIME)));
```
##########
cpp-ch/local-engine/Parser/RelParsers/MergeTreeRelParser.h:
##########
@@ -43,6 +46,11 @@ using ReplaceDeltaNodeFunc =
std::function<void(DB::ActionsDAG &, const MergeTre
void replaceInputFileNameNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context);
void replaceFilePathNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context);
void replaceFileNameNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context);
+void replaceFileSizeNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context);
+void replaceFileBlockStartNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context);
+void replaceFileBlockLengthNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context);
+void replaceFileModificationTimeNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context);
+void replaceDeltaInternalRowDeletedNode(DB::ActionsDAG & actions_dag, const
MergeTreeTableInstance & merge_tree_table, DB::ContextPtr context);
Review Comment:
[nitpick] Consider adding a brief comment above this method (and the other
`replace*Node` functions) to describe what virtual column it injects and why.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
