You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "WillAyd (via GitHub)" <gi...@apache.org> on 2023/06/14 20:21:10 UTC
[GitHub] [arrow-adbc] WillAyd opened a new pull request, #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
WillAyd opened a new pull request, #799:
URL: https://github.com/apache/arrow-adbc/pull/799
(no comment)
--
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 #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799
--
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 #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1230961607
##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -421,6 +437,80 @@ TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsForeignKey) {
}
}
+TEST_F(PostgresConnectionTest, GetObjectsTableTypesFilter) {
+ ASSERT_THAT(AdbcConnectionNew(&connection, &error), IsOkStatus(&error));
+ ASSERT_THAT(AdbcConnectionInit(&connection, &database, &error), IsOkStatus(&error));
+
+ if (!quirks()->supports_get_objects()) {
+ GTEST_SKIP();
+ }
+
+ ASSERT_THAT(quirks()->DropView(&connection, "adbc_table_types_view_test", &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(quirks()->DropTable(&connection, "adbc_table_types_table_test", &error),
+ IsOkStatus(&error));
+
+ struct AdbcStatement statement;
+ ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
+ {
+ ASSERT_THAT(
+ AdbcStatementSetSqlQuery(
+ &statement, "CREATE TABLE adbc_table_types_table_test (id1 INT, id2 INT)",
+ &error),
+ IsOkStatus(&error));
+ adbc_validation::StreamReader reader;
+ ASSERT_THAT(AdbcStatementExecuteQuery(&statement, &reader.stream.value,
+ &reader.rows_affected, &error),
+ IsOkStatus(&error));
+ ASSERT_EQ(reader.rows_affected, 0);
+ ASSERT_NO_FATAL_FAILURE(reader.GetSchema());
+ ASSERT_NO_FATAL_FAILURE(reader.Next());
+ ASSERT_EQ(reader.array->release, nullptr);
+ }
+ ASSERT_THAT(AdbcStatementRelease(&statement, &error), IsOkStatus(&error));
Review Comment:
Same here, `Handle<AdbcStatement>` should simplify things
##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -63,6 +63,24 @@ class PostgresQuirks : public adbc_validation::DriverQuirks {
return status;
}
+ AdbcStatusCode DropView(struct AdbcConnection* connection, const std::string& name,
+ struct AdbcError* error) const override {
+ struct AdbcStatement statement;
+ std::memset(&statement, 0, sizeof(statement));
+ AdbcStatusCode status = AdbcStatementNew(connection, &statement, error);
+ if (status != ADBC_STATUS_OK) return status;
+
+ std::string query = "DROP VIEW IF EXISTS " + name;
+ status = AdbcStatementSetSqlQuery(&statement, query.c_str(), error);
+ if (status != ADBC_STATUS_OK) {
+ std::ignore = AdbcStatementRelease(&statement, error);
Review Comment:
Use `Handle<AdbcStatement>` to avoid this sort of thing
##########
c/validation/adbc_validation.h:
##########
@@ -47,6 +47,13 @@ class DriverQuirks {
return ADBC_STATUS_OK;
}
+ /// \brief Drop the given view. Used by tests to reset state.
+ virtual AdbcStatusCode DropView(struct AdbcConnection* connection,
+ const std::string& name,
+ struct AdbcError* error) const {
+ return ADBC_STATUS_OK;
Review Comment:
return `NOT_IMPLEMENTED` for the default
##########
c/driver/postgresql/connection.cc:
##########
@@ -365,6 +370,45 @@ class PqGetObjectsHelper {
params.push_back(std::string(table_name_));
}
+ if (table_types_ != NULL) {
Review Comment:
Use `nullptr` in C++
##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
na_error_ = {0};
}
+ static const inline std::unordered_map<std::string, std::string> PgTableTypes = {
Review Comment:
nit: I don't think `inline` does much here, and I think coding convention would make this `kPgTableTypes`?
--
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] WillAyd commented on a diff in pull request #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1231441238
##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
na_error_ = {0};
}
+ static const inline std::unordered_map<std::string, std::string> PgTableTypes = {
Review Comment:
Ah that's great info. Arrow core is C++17 right? Thought that was the case maybe misinterpreted
--
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] WillAyd commented on a diff in pull request #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1230123782
##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
na_error_ = {0};
}
+ static const inline std::unordered_map<std::string, std::string> PgTableTypes = {
Review Comment:
I've attached this to PqGetObjectsHelper for now, though I think could also just be a static constant in the module
##########
c/driver/postgresql/connection.cc:
##########
@@ -365,6 +370,45 @@ class PqGetObjectsHelper {
params.push_back(std::string(table_name_));
}
+ if (table_types_ != NULL) {
+ std::vector<std::string> table_type_filter;
+ const char** table_types = table_types_;
+ while (*table_types != NULL) {
+ auto table_type_str = std::string(*table_types);
+ if (auto search = PqGetObjectsHelper::PgTableTypes.find(table_type_str);
+ search != PqGetObjectsHelper::PgTableTypes.end()) {
+ table_type_filter.push_back(search->second);
+ }
+ table_types++;
+ }
+
+ if (!table_type_filter.empty()) {
+ std::ostringstream oss;
+ bool first = true;
+ oss << "(";
+ for (const auto& str : table_type_filter) {
+ if (!first) {
+ oss << ", ";
+ }
+ oss << "'" << str << "'";
+ first = false;
+ }
+ oss << ")";
+
+ if (StringBuilderAppend(&query, "%s%s", " AND c.relkind IN ",
Review Comment:
This does not use a parameter/prepared statement. My reasoning is that it is internally controlled which arguments end up being in the filter, and it was simpler to attach literally instead of keeping track of / enumerating all possible parameters in the statement
##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -63,6 +63,24 @@ class PostgresQuirks : public adbc_validation::DriverQuirks {
return status;
}
+ AdbcStatusCode DropView(struct AdbcConnection* connection, const std::string& name,
Review Comment:
Instead of a new method I think could also parametrize `DropTable` to accept a constant table type argument. That would be more consistent with other method names that refer to everything as `Table`
--
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 #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1231439862
##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
na_error_ = {0};
}
+ static const inline std::unordered_map<std::string, std::string> PgTableTypes = {
Review Comment:
I think you can just put it outside the class then (shouldn't matter since this is in a cc file and not a header). Plus we've been trying to avoid C++17 for R's sake (though apparently we don't enforce that!), since some R distributions still ship with only C++11 support
--
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] paleolimbot commented on a diff in pull request #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1233337312
##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
na_error_ = {0};
}
+ static const inline std::unordered_map<std::string, std::string> PgTableTypes = {
Review Comment:
Yes, sorry 😬 . There are two platforms that are not uncommon for R where this comes up (R 3.6 on Windows and Centos7/RHEL7 with the default compilers). If you really have to use C++17 it is OK; however, it does make my life a little easier if it can be avoided for another year or so.
--
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] WillAyd commented on pull request #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#issuecomment-1592050168
Don't think the R failure is related
--
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] WillAyd commented on a diff in pull request #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1231437551
##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
na_error_ = {0};
}
+ static const inline std::unordered_map<std::string, std::string> PgTableTypes = {
Review Comment:
Without the `inline` keyword I get the following error:
```
/home/willayd/clones/arrow-adbc/c/driver/postgresql/connection.cc:189:61: error: in-class initialization of static data member ‘const std::unordered_map<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> > {anonymous}::PqGetObjectsHelper::PgTableTypes’ of non-literal type
189 | static const std::unordered_map<std::string, std::string> PgTableTypes = {
| ^~~~~~~~~~~~
/home/willayd/clones/arrow-adbc/c/driver/postgresql/connection.cc:191:79: error: temporary of non-literal type ‘const std::unordered_map<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> >’ in a constant expression
191 | {"toast_table", "t"}, {"foreign_table", "f"}, {"partitioned_table", "p"}};
```
AFAICT this is a C++17 feature
https://stackoverflow.com/a/38043566/621736
--
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 #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1231443839
##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
na_error_ = {0};
}
+ static const inline std::unordered_map<std::string, std::string> PgTableTypes = {
Review Comment:
Arrow C++ is C++17 indeed; I think what happened here is that I intended for C++17 (or really, didn't really think about it), but we didn't end up using anything from C++14 or 17 at all (except in test code), and it turned out to be more convenient to stick to C++11 in non-test code for now to help R. @paleolimbot can explain the R part better than I.
--
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] WillAyd commented on a diff in pull request #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1231492721
##########
c/validation/adbc_validation.h:
##########
@@ -47,6 +47,13 @@ class DriverQuirks {
return ADBC_STATUS_OK;
}
+ /// \brief Drop the given view. Used by tests to reset state.
+ virtual AdbcStatusCode DropView(struct AdbcConnection* connection,
+ const std::string& name,
+ struct AdbcError* error) const {
+ return ADBC_STATUS_OK;
Review Comment:
Was going to try and make all of the virtual methods here return NOT_IMPLEMENTED; doing so for the DropTable method yields some sqlite test errors though. Guessing the quirks checking in that test module is off - something for later investigation
```sh
/home/willayd/clones/arrow-adbc/c/validation/adbc_validation.cc:1943: Failure
Value of: quirks()->DropTable(&connection, "bulk_ingest", &error)
Expected: is 0 (OK)
Actual: '\x2' (2) (of type unsigned char), 2 (NOT_IMPLEMENTED)
```
--
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 #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1233356120
##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
na_error_ = {0};
}
+ static const inline std::unordered_map<std::string, std::string> PgTableTypes = {
Review Comment:
I filed https://github.com/apache/arrow-adbc/issues/815 to make sure we set this explicitly
--
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