lidavidm commented on code in PR #2254:
URL: https://github.com/apache/arrow-adbc/pull/2254#discussion_r1804191047
##########
c/validation/adbc_validation_connection.cc:
##########
@@ -744,27 +742,29 @@ void ConnectionTest::TestMetadataGetObjectsColumns() {
struct TestCase {
std::optional<std::string> filter;
- std::vector<std::string> column_names;
- std::vector<int32_t> ordinal_positions;
+ std::vector<std::pair<std::string, int32_t>> columns;
Review Comment:
Assuming the pair is meant to represent ordinal positions, can we leave a
quick comment to that effect?
##########
c/validation/adbc_validation_connection.cc:
##########
@@ -701,9 +701,7 @@ void ConnectionTest::TestMetadataGetObjectsTablesTypes() {
db_schemas_index <
ArrowArrayViewListChildOffset(catalog_db_schemas_list, row + 1);
db_schemas_index++) {
- ASSERT_FALSE(ArrowArrayViewIsNull(db_schema_tables_list,
db_schemas_index))
- << "Row " << row << " should have non-null db_schema_tables";
-
+ // db_schema_tables should either be null or an empty list
Review Comment:
I think the intent was that it would only be null if the depth is set to
exclude tables, otherwise it should at least be an empty list
##########
c/validation/adbc_validation_connection.cc:
##########
@@ -847,8 +846,11 @@ void ConnectionTest::TestMetadataGetObjectsColumns() {
} while (reader.array->release);
ASSERT_TRUE(found_expected_table) << "Did (not) find table in metadata";
- ASSERT_EQ(test_case.column_names, column_names);
- ASSERT_EQ(test_case.ordinal_positions, ordinal_positions);
+ // metadata columns do not guarantee the order they are returned in, we can
+ // avoid the test being flakey by sorting the column names we found
+ std::sort(columns.begin(), columns.end(),
+ [](const auto& a, const auto& b) -> bool { return a.first <
b.first; });
+ ASSERT_EQ(test_case.columns, columns);
Review Comment:
`ASSERT_THAT(columns,
testing::UnorderedElementsAreArray(test_case.columns))`?
##########
c/validation/adbc_validation_statement.cc:
##########
@@ -2180,15 +2180,15 @@ void StatementTest::TestSqlBind() {
ASSERT_THAT(
AdbcStatementSetSqlQuery(
- &statement, "SELECT * FROM bindtest ORDER BY \"col1\" ASC NULLS
FIRST", &error),
+ &statement, "SELECT * FROM bindtest ORDER BY col1 ASC NULLS FIRST",
&error),
Review Comment:
Do we perhaps need a quirk for escaping column names?
(I also wouldn't be opposed to trying to make these tests more
data-driven...I should go find time to sketch it out)
--
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]
