Skip to content

Conversation

@chernser
Copy link
Contributor

Summary

  • Fixed problem with duplicate column names
  • Last duplicate column index is remembered in index-by-name map.
  • Table Schema will return list of all columns anyway

Closes #2459
Closes #2336

Checklist

Delete items not relevant to your PR:

  • Closes #
  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG
  • For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@chernser chernser marked this pull request as ready for review October 25, 2025 05:11
@chernser chernser added this to the 0.9.3 milestone Oct 25, 2025
Copy link

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 To request another review, post a new comment with "/windsurf-review".

Comment on lines +2231 to +2232
client.execute("DROP TABLE IF EXISTS test_duplicate_column_names_1").get().close();
client.execute("DROP TABLE IF EXISTS test_duplicate_column_names_2").get().close();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table names in the DROP statements don't match the CREATE statements. The DROP statements use test_duplicate_column_names_1 and test_duplicate_column_names_2 (with underscores), but the CREATE statements use test_duplicate_column_names1 and test_duplicate_column_names2 (without underscores).

Suggested change
client.execute("DROP TABLE IF EXISTS test_duplicate_column_names_1").get().close();
client.execute("DROP TABLE IF EXISTS test_duplicate_column_names_2").get().close();
client.execute("DROP TABLE IF EXISTS test_duplicate_column_names1").get().close();
client.execute("DROP TABLE IF EXISTS test_duplicate_column_names2").get().close();

Also, consider adding similar DROP statements at the end of the test to clean up the created tables.

colIndexMapBuilder.put(this.columns.get(i).getColumnName(), i);
}
this.colIndex = colIndexMapBuilder.build();
this.colIndex = colIndexMapBuilder.buildKeepingLast();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change addresses the duplicate column names issue by keeping the last occurrence of a column name in the index map. Consider adding a comment explaining this behavior, as it's an important detail for users of this API to understand that when duplicate column names exist, only the last one will be accessible via name-based lookups.

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant