felipepessoto commented on code in PR #13136:
URL: https://github.com/apache/gluten/pull/13136#discussion_r4405630267
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -1656,34 +1656,44 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
auto names = colNameList;
auto types = veloxTypeList;
- // The columns we project from the file.
+ // The logical scan schema includes columns supplied by the connector, not
just the file.
auto baseSchema = ROW(std::move(names), std::move(types));
- // The columns present in the table, if not available default to the
baseSchema.
- auto tableSchema = splitInfo->tableSchema ? splitInfo->tableSchema :
baseSchema;
- // Build dataColumns from tableSchema, excluding partition columns.
// HiveTableHandle::dataColumns() is used as fileSchema for the reader.
- // Partition columns should not be validated against the file's physical
types
- // (their values come from the partition path, not from the file).
- std::unordered_set<std::string> partitionColNames;
- for (int idx = 0; idx < colNameList.size(); idx++) {
- if (columnTypes[idx] == ColumnType::kPartitionKey) {
- partitionColNames.insert(colNameList[idx]);
- }
- }
RowTypePtr dataColumns;
- if (partitionColNames.empty()) {
- dataColumns = tableSchema;
- } else {
+ if (!splitInfo->tableSchema) {
std::vector<std::string> dataColNames;
std::vector<TypePtr> dataColTypes;
- for (int idx = 0; idx < tableSchema->size(); idx++) {
- if (partitionColNames.find(tableSchema->nameOf(idx)) ==
partitionColNames.end()) {
- dataColNames.push_back(tableSchema->nameOf(idx));
- dataColTypes.push_back(tableSchema->childAt(idx));
+ for (int idx = 0; idx < colNameList.size(); idx++) {
+ if (columnTypes[idx] == ColumnType::kRegular) {
+ dataColNames.push_back(colNameList[idx]);
+ dataColTypes.push_back(veloxTypeList[idx]);
}
}
dataColumns = ROW(std::move(dataColNames), std::move(dataColTypes));
+ } else {
+ // An explicit schema describes physical fields and their ordinals. A field
+ // must not be removed just because a synthetic output has the same name.
+ const auto& tableSchema = splitInfo->tableSchema;
+ std::unordered_set<std::string> partitionColNames;
+ for (int idx = 0; idx < colNameList.size(); idx++) {
+ if (columnTypes[idx] == ColumnType::kPartitionKey) {
+ partitionColNames.insert(colNameList[idx]);
+ }
+ }
+ if (partitionColNames.empty()) {
+ dataColumns = tableSchema;
+ } else {
+ std::vector<std::string> dataColNames;
+ std::vector<TypePtr> dataColTypes;
+ for (int idx = 0; idx < tableSchema->size(); idx++) {
+ if (partitionColNames.find(tableSchema->nameOf(idx)) ==
partitionColNames.end()) {
+ dataColNames.push_back(tableSchema->nameOf(idx));
+ dataColTypes.push_back(tableSchema->childAt(idx));
+ }
+ }
+ dataColumns = ROW(std::move(dataColNames), std::move(dataColTypes));
+ }
Review Comment:
The explicit-schema partition exclusion is unchanged from `main`; this PR
changes only how the schema is derived when no explicit schema is supplied. The
normal file-scan caller passes `relation.dataSchema` as the file schema and
supplies `relation.partitionSchema` separately, so directory partition values
are not physical field positions. Keeping the existing policy here avoids
changing partition behavior as part of the metadata fix. A genuinely physical
field sharing a partition name would need a separate reproduction; this diff
does not introduce that case.
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -1656,34 +1656,44 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
auto names = colNameList;
auto types = veloxTypeList;
- // The columns we project from the file.
+ // The logical scan schema includes columns supplied by the connector, not
just the file.
auto baseSchema = ROW(std::move(names), std::move(types));
- // The columns present in the table, if not available default to the
baseSchema.
- auto tableSchema = splitInfo->tableSchema ? splitInfo->tableSchema :
baseSchema;
- // Build dataColumns from tableSchema, excluding partition columns.
// HiveTableHandle::dataColumns() is used as fileSchema for the reader.
- // Partition columns should not be validated against the file's physical
types
- // (their values come from the partition path, not from the file).
- std::unordered_set<std::string> partitionColNames;
- for (int idx = 0; idx < colNameList.size(); idx++) {
- if (columnTypes[idx] == ColumnType::kPartitionKey) {
- partitionColNames.insert(colNameList[idx]);
- }
- }
RowTypePtr dataColumns;
- if (partitionColNames.empty()) {
- dataColumns = tableSchema;
- } else {
+ if (!splitInfo->tableSchema) {
std::vector<std::string> dataColNames;
std::vector<TypePtr> dataColTypes;
- for (int idx = 0; idx < tableSchema->size(); idx++) {
- if (partitionColNames.find(tableSchema->nameOf(idx)) ==
partitionColNames.end()) {
- dataColNames.push_back(tableSchema->nameOf(idx));
- dataColTypes.push_back(tableSchema->childAt(idx));
+ for (int idx = 0; idx < colNameList.size(); idx++) {
+ if (columnTypes[idx] == ColumnType::kRegular) {
Review Comment:
Keeping these branches separate because they encode different contracts: the
derived schema selects regular scan slots by role/index, whereas an explicit
schema preserves physical fields/order and applies only the existing
partition-name exclusion. A shared filtering helper would reduce a few
collection lines but obscure the distinction that this fix needs to preserve.
--
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]
