You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2023/05/10 20:44:31 UTC
[GitHub] [arrow-adbc] zeroshade opened a new pull request, #677: fix(c/driver/snowflake): fix validation test failures
zeroshade opened a new pull request, #677:
URL: https://github.com/apache/arrow-adbc/pull/677
Fixes #667
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190394851
##########
go/adbc/driver/snowflake/record_reader.go:
##########
@@ -219,7 +219,7 @@ func getTransformer(sc *arrow.Schema, ld gosnowflake.ArrowStreamLoader) (*arrow.
default:
transformers[i] = identCol
}
-
+ f.Name = strings.ToLower(f.Name)
Review Comment:
Okay, i'll leave it returning upper case and then adjust CompareSchema to check case-insensitively
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm merged pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190391864
##########
go/adbc/driver/snowflake/record_reader.go:
##########
@@ -219,7 +219,7 @@ func getTransformer(sc *arrow.Schema, ld gosnowflake.ArrowStreamLoader) (*arrow.
default:
transformers[i] = identCol
}
-
+ f.Name = strings.ToLower(f.Name)
Review Comment:
I'm not sure. Unfortunately we're probably gonna run into this more as we add more drivers. Snowflake currently returns *everything* unconditionally upper-cased as far as I can tell. While our tests expect things to be lower cased. I personally prefer consistency so I like the idea of saying we lower case the field names..... but I dunno.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190392911
##########
c/validation/adbc_validation.cc:
##########
@@ -1248,11 +1258,10 @@ void StatementTest::TestSqlIngestSample() {
IsOkStatus(&error));
ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
- ASSERT_THAT(
- AdbcStatementSetSqlQuery(
- &statement, "SELECT * FROM bulk_ingest ORDER BY \"int64s\" ASC NULLS FIRST",
- &error),
- IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementSetSqlQuery(
+ &statement, "SELECT * FROM bulk_ingest ORDER BY int64s ASC NULLS FIRST",
Review Comment:
Er, yes. But the point stands; we should start making the suite a little more flexible.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190392742
##########
go/adbc/driver/snowflake/record_reader.go:
##########
@@ -219,7 +219,7 @@ func getTransformer(sc *arrow.Schema, ld gosnowflake.ArrowStreamLoader) (*arrow.
default:
transformers[i] = identCol
}
-
+ f.Name = strings.ToLower(f.Name)
Review Comment:
Hmm, I'd rather return what the database gives us unchanged as much as possible. The validation suite will need to get more flexible.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190386701
##########
c/validation/adbc_validation.cc:
##########
@@ -1248,11 +1258,10 @@ void StatementTest::TestSqlIngestSample() {
IsOkStatus(&error));
ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
- ASSERT_THAT(
- AdbcStatementSetSqlQuery(
- &statement, "SELECT * FROM bulk_ingest ORDER BY \"int64s\" ASC NULLS FIRST",
- &error),
- IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementSetSqlQuery(
+ &statement, "SELECT * FROM bulk_ingest ORDER BY int64s ASC NULLS FIRST",
Review Comment:
we probably need a table-name-quote quirk or else just make these queries provided as part of the quirks, I expect
##########
go/adbc/driver/snowflake/record_reader.go:
##########
@@ -219,7 +219,7 @@ func getTransformer(sc *arrow.Schema, ld gosnowflake.ArrowStreamLoader) (*arrow.
default:
transformers[i] = identCol
}
-
+ f.Name = strings.ToLower(f.Name)
Review Comment:
Hmm, do we want to unconditionally do this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190394567
##########
c/validation/adbc_validation.cc:
##########
@@ -1248,11 +1258,10 @@ void StatementTest::TestSqlIngestSample() {
IsOkStatus(&error));
ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
- ASSERT_THAT(
- AdbcStatementSetSqlQuery(
- &statement, "SELECT * FROM bulk_ingest ORDER BY \"int64s\" ASC NULLS FIRST",
- &error),
- IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementSetSqlQuery(
+ &statement, "SELECT * FROM bulk_ingest ORDER BY int64s ASC NULLS FIRST",
Review Comment:
true and fair. I'd rather keep that for a separate PR if that works for you?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190395989
##########
c/validation/adbc_validation.cc:
##########
@@ -1248,11 +1258,10 @@ void StatementTest::TestSqlIngestSample() {
IsOkStatus(&error));
ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
- ASSERT_THAT(
- AdbcStatementSetSqlQuery(
- &statement, "SELECT * FROM bulk_ingest ORDER BY \"int64s\" ASC NULLS FIRST",
- &error),
- IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementSetSqlQuery(
+ &statement, "SELECT * FROM bulk_ingest ORDER BY int64s ASC NULLS FIRST",
Review Comment:
Yup - sounds good (do you mind filing an issue though?)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190410053
##########
c/validation/adbc_validation.cc:
##########
@@ -1248,11 +1258,10 @@ void StatementTest::TestSqlIngestSample() {
IsOkStatus(&error));
ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
- ASSERT_THAT(
- AdbcStatementSetSqlQuery(
- &statement, "SELECT * FROM bulk_ingest ORDER BY \"int64s\" ASC NULLS FIRST",
- &error),
- IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementSetSqlQuery(
+ &statement, "SELECT * FROM bulk_ingest ORDER BY int64s ASC NULLS FIRST",
Review Comment:
Filed https://github.com/apache/arrow-adbc/issues/678
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190392134
##########
c/validation/adbc_validation.cc:
##########
@@ -1248,11 +1258,10 @@ void StatementTest::TestSqlIngestSample() {
IsOkStatus(&error));
ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
- ASSERT_THAT(
- AdbcStatementSetSqlQuery(
- &statement, "SELECT * FROM bulk_ingest ORDER BY \"int64s\" ASC NULLS FIRST",
- &error),
- IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementSetSqlQuery(
+ &statement, "SELECT * FROM bulk_ingest ORDER BY int64s ASC NULLS FIRST",
Review Comment:
well in this case it's not the table name, it's the column name that's the issue. Because snowflake unconditionally upper-cases things which aren't quoted, when you add double quotes, it keeps it lower cased and then it can't find the column because `int64s` != `INT64S`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #677: fix(c/driver/snowflake): fix validation test failures
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #677:
URL: https://github.com/apache/arrow-adbc/pull/677#discussion_r1190392134
##########
c/validation/adbc_validation.cc:
##########
@@ -1248,11 +1258,10 @@ void StatementTest::TestSqlIngestSample() {
IsOkStatus(&error));
ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
- ASSERT_THAT(
- AdbcStatementSetSqlQuery(
- &statement, "SELECT * FROM bulk_ingest ORDER BY \"int64s\" ASC NULLS FIRST",
- &error),
- IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementSetSqlQuery(
+ &statement, "SELECT * FROM bulk_ingest ORDER BY int64s ASC NULLS FIRST",
Review Comment:
well in this case it's not the table name, it's the column name that's the issue.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org