diff --git a/src/binder/bind/bind_file_scan.cpp b/src/binder/bind/bind_file_scan.cpp index 8297f1f5c2..e9a533a09c 100644 --- a/src/binder/bind/bind_file_scan.cpp +++ b/src/binder/bind/bind_file_scan.cpp @@ -198,9 +198,9 @@ std::unique_ptr Binder::bindQueryScanSource(const BaseScanS throw BinderException(std::format("Query returns {} columns but {} columns were expected.", columns.size(), columnNames.size())); } - for (auto i = 0u; i < columns.size(); ++i) { - columns[i]->setAlias(columnNames[i]); - } + // The columns are matched to columnNames by position when the COPY operator is bound. They + // must not be renamed here: a RETURN list can bind the same Expression object more than once + // (e.g. RETURN i, i), and setAlias would leave that shared object holding one name only. auto scanInfo = BoundQueryScanSourceInfo(bindParsingOptions(options)); return std::make_unique(std::move(boundStatement), std::move(scanInfo)); } diff --git a/src/binder/bind/copy/bind_copy_from.cpp b/src/binder/bind/copy/bind_copy_from.cpp index a2715cabc8..fa4063fe6a 100644 --- a/src/binder/bind/copy/bind_copy_from.cpp +++ b/src/binder/bind/copy/bind_copy_from.cpp @@ -116,18 +116,26 @@ static void bindExpectedRelColumns(const RelGroupCatalogEntry& entry, const CopyFromColumnInfo& info, std::vector& columnNames, std::vector& columnTypes); +// Returns the source column that feeds a table property. A scan source names its leading columns +// from expectedColumnNames by position, so the position decides which column a property reads. +// The name of the bound expression cannot be used for this: a query source may return one shared +// Expression object for several columns (e.g. RETURN i, i). Columns past the expected ones carry +// copy warnings. A source bound without expected names, such as a table whose only column is +// SERIAL, keeps the names it read from the file and is matched by those names. static std::pair> matchColumnExpression( - const expression_vector& columns, const PropertyDefinition& property, - ExpressionBinder& expressionBinder) { - for (auto& column : columns) { - if (property.getName() == column->toString()) { - if (column->dataType == property.getType()) { - return {ColumnEvaluateType::REFERENCE, column}; - } else { - return {ColumnEvaluateType::CAST, - expressionBinder.forceCast(column, property.getType())}; - } + const expression_vector& columns, const std::vector& expectedColumnNames, + const PropertyDefinition& property, ExpressionBinder& expressionBinder) { + for (auto i = 0u; i < columns.size(); ++i) { + auto columnName = + i < expectedColumnNames.size() ? expectedColumnNames[i] : columns[i]->toString(); + if (property.getName() != columnName) { + continue; + } + auto& column = columns[i]; + if (column->dataType == property.getType()) { + return {ColumnEvaluateType::REFERENCE, column}; } + return {ColumnEvaluateType::CAST, expressionBinder.forceCast(column, property.getType())}; } return {ColumnEvaluateType::DEFAULT, expressionBinder.bindExpression(*property.defaultExpr)}; } @@ -163,8 +171,8 @@ BoundCopyFromInfo Binder::bindCopyNodeFromInfo(std::string tableName, expression_vector columns; std::vector evaluateTypes; for (auto& property : properties) { - auto [evaluateType, column] = - matchColumnExpression(boundSource->getColumns(), property, expressionBinder); + auto [evaluateType, column] = matchColumnExpression(boundSource->getColumns(), + expectedColumnNames, property, expressionBinder); columns.push_back(column); evaluateTypes.push_back(evaluateType); } @@ -247,8 +255,8 @@ BoundCopyFromInfo Binder::bindCopyRelFromInfo(std::string tableName, ColumnEvaluateType::REFERENCE, ColumnEvaluateType::REFERENCE}; for (auto i = 1u; i < properties.size(); ++i) { // skip internal ID auto& property = properties[i]; - auto [evaluateType, column] = - matchColumnExpression(boundSource->getColumns(), property, expressionBinder); + auto [evaluateType, column] = matchColumnExpression(boundSource->getColumns(), + expectedColumnNames, property, expressionBinder); columnExprs.push_back(column); evaluateTypes.push_back(evaluateType); } diff --git a/test/test_files/issue/issue1365.test b/test/test_files/issue/issue1365.test new file mode 100644 index 0000000000..1d51fbd7af --- /dev/null +++ b/test/test_files/issue/issue1365.test @@ -0,0 +1,100 @@ +-DATASET CSV empty + +-- + +-CASE Issue1365CopyFromSubqueryDuplicateVariable +# Reproduction for github issue #1365: +# COPY FROM (subquery) whose RETURN reuses a variable (RETURN i, i, i) +# failed with a spurious NULL primary key, and a reused variable in a non-key +# position was inserted as NULL instead of its value. +# Root cause: Binder::bindQueryScanSource renamed the subquery result columns by +# calling setAlias on the shared Expression objects. A variable bound twice is +# one object, so it kept only the last expected name, and the copy binder - which +# matched table properties to columns through that name - fell back to the column +# default. Result columns are now mapped positionally against the expected column +# names, so no bound expression is mutated. + +-STATEMENT CREATE NODE TABLE q3(ID INT64 PRIMARY KEY, a INT64, b INT64); +---- ok + +-STATEMENT COPY q3 FROM (UNWIND range(20, 22) AS i RETURN i, i, i); +---- ok + +-STATEMENT MATCH (n:q3) RETURN n.ID, n.a, n.b ORDER BY n.ID; +---- 3 +20|20|20 +21|21|21 +22|22|22 + +# The same failure with a string column, reported while testing PR #1350. +-STATEMENT CREATE NODE TABLE artist(ID INT64, name STRING, rank INT64, PRIMARY KEY(ID)); +---- ok + +-STATEMENT COPY artist FROM (UNWIND range(20, 22) AS i RETURN i, 'Grace', i); +---- ok + +-STATEMENT MATCH (n:artist) RETURN n.ID, n.name, n.rank ORDER BY n.ID; +---- 3 +20|Grace|20 +21|Grace|21 +22|Grace|22 + +# The workarounds from the issue, where every RETURN expression is a distinct +# object, already worked and must keep working. +-STATEMENT COPY q3 FROM (UNWIND range(30, 31) AS i RETURN i, i + 0, i + 1); +---- ok + +-STATEMENT MATCH (n:q3) RETURN n.ID, n.a, n.b ORDER BY n.ID; +---- 5 +20|20|20 +21|21|21 +22|22|22 +30|30|31 +31|31|32 + +# An aliased RETURN list keeps mapping positionally onto the table schema. +-STATEMENT COPY q3 FROM (UNWIND range(40, 40) AS i RETURN i AS x, i + 1 AS y, i + 2 AS z); +---- ok + +-STATEMENT MATCH (n:q3) RETURN n.ID, n.a, n.b ORDER BY n.ID; +---- 6 +20|20|20 +21|21|21 +22|22|22 +30|30|31 +31|31|32 +40|41|42 + +# A column list assigns the source positions to properties, so the property +# order is not the source order. Before the fix the reused variable kept only +# the last expected name and the other property took its default (NULL). +-STATEMENT CREATE NODE TABLE r2(ID INT64 PRIMARY KEY, a INT64); +---- ok + +-STATEMENT COPY r2 (a, ID) FROM (UNWIND range(1, 2) AS i RETURN i, i); +---- ok + +-STATEMENT MATCH (n:r2) RETURN n.ID, n.a ORDER BY n.ID; +---- 2 +1|1 +2|2 + +# Relationship copy: source and destination are taken by position, the +# remaining properties by name. +-STATEMENT CREATE NODE TABLE person(ID INT64 PRIMARY KEY); +---- ok + +-STATEMENT CREATE REL TABLE knows(FROM person TO person, w INT64, k INT64); +---- ok + +-STATEMENT COPY person FROM (UNWIND range(1, 3) AS i RETURN i); +---- ok + +-STATEMENT COPY knows FROM (UNWIND range(1, 3) AS i RETURN i, i, i, i); +---- ok + +-STATEMENT MATCH (a:person)-[e:knows]->(b:person) RETURN e.w, e.k ORDER BY e.w, e.k; +---- 3 +1|1 +2|2 +3|3