Postgres dv2: add indexes #34225
Postgres dv2: add indexes #34225
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
octavia-squidington-iii
added
area/connectors
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
edgao
force-pushed
the
edgao/dv2/postgres/indexes
branch
from
92bab23 to
a5b75fd
Compare
# Conflicts: # airbyte-integrations/connectors/destination-bigquery/build.gradle # airbyte-integrations/connectors/destination-redshift/build.gradle # airbyte-integrations/connectors/destination-snowflake/build.gradle # docs/integrations/destinations/bigquery.md # docs/integrations/destinations/snowflake.md
|
... should |
| @Override | ||
| protected List<String> postCreateTableQueries(final String schemaName, final String tableName) { | ||
| if (TypingAndDedupingFlag.isDestinationV2()) { | ||
| return List.of( | ||
| "CREATE INDEX IF NOT EXISTS " + tableName + "_raw_id" + " ON " + tableName + "(_airbyte_raw_id)", | ||
| "CREATE INDEX IF NOT EXISTS " + tableName + "_extracted_at" + " ON " + tableName + "(_airbyte_extracted_at)", | ||
| "CREATE INDEX IF NOT EXISTS " + tableName + "_loaded_at" + " ON " + tableName + "(_airbyte_loaded_at, _airbyte_extracted_at)" | ||
| ); | ||
| } else { | ||
| return Collections.emptyList(); | ||
| } | ||
| } |
There was a problem hiding this comment.
There doesn't seem to be any way of specifying an index in the CREATE TABLE syntax.
TIL!
There was a problem hiding this comment.
There are all sorts if indexes we could be creating (Btree, hash, etc), but I think "unspecified" index type like this is the right thing to do, as it will fallback to the default for the database/schema
There was a problem hiding this comment.
It would also be reasonable to make the index on _raw_id UNIQUE, but we make this field and we can trust UUID to be unique, so I don't think we need this validation. Adding a UNIQUE index would also make writes slower, and we don't want that.
There was a problem hiding this comment.
these are good thoughts which I definitely did not think :P
I'll copypaste it into our code as an actual comment for easier future reference
Co-authored-by: maxi297 <maxime@airbyte.io>
octavia-squidington-iii
added
area/documentation
I think eventually yes, but not literally today. If any beta users want us to change things indexes, I feel ok asking them to manually drop all their tables? Having autodetections feels like a "after we release to GA and we want to change our indexes" feature it's probably not a huge lift (something like fetch jdbc metadata -> dump into the TableDefinition struct -> diff appropriately) and is a pure bolt-on addition in the future, so I feel ok deferring it for now? |
|
ffs. bad merge. closing this pr and opening a new one |
see https://github.com/airbytehq/airbyte-internal-issues/issues/2423 - does not close it b/c that ticket also covers mysql.
based on top of #34177 (at some point I really should try out graphite...)
We'll need to manually drop the existing test tables after releasing this (the existing JdbcDestinationHandler can't detect a change in indexes, and there's no table-change detection logic for raw tables). That's OK, because we haven't released the beta yet, so we're the only users.
Also minorly refactors the final table index creation code - instead of exposing a createIndex method in SqlGenerator, just have PostgresSqlGenerator override createTable to append more statements. I think that's better (reduces the scope of the base JdbcSqlGenerator class)? But open to reverting it if people disagree.