Skip to content

Infer sync statements for raw tables#164

Draft
simolus3 wants to merge 4 commits intoraw-table-triggersfrom
infer-raw-table-statements
Draft

Infer sync statements for raw tables#164
simolus3 wants to merge 4 commits intoraw-table-triggersfrom
infer-raw-table-statements

Conversation

@simolus3
Copy link
Contributor

This builds on top of the new raw table options introduced in #163.

This makes the put and delete statements set on raw tables optional. If they're omitted, the sync_local step of each sync will attempt to infer them by looking at the actual columns of the table:

  1. The id column will be set to the affected row id.
  2. Other columns will be bound to extracted JSON values of the same name.
    • Columns marked as local-only are ignored.

Inferred statements are cached until pragma schema_version returns a new value.

For now, this does not support a dedicated _rest column that would be inferred by default. Users interested in that would have to use explicit statements instead.

Copy link
Contributor

@rkistner rkistner left a comment

Choose a reason for hiding this comment

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

Is it feasible to add a test for the generated SQL? Even if it's just a Rust unit test rather than an "end-to-end" Dart one.

final table = {
'name': 'users',
'table_name': 'local_users',
'local_only_columns': ['local'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering, should we perhaps invert this? Instead of specifying the local_only_columns, rather specify a specific list of columns to sync?

That would be closer to what we have with JSON tables (except we don't need the types).
And we could still default to inferring all the columns if not specified.

let mut buffer = SqlBuffer::new();
let mut params = vec![];

buffer.push_str("INSERT OR REPLACE INTO ");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we perhaps change this to use upsert statements instead?

This is specifically required when using local-only columns - see https://docs.powersync.com/client-sdks/advanced/raw-tables#use-upsert-instead-of-insert-or-replace

However, even without local-only columns it could be better, especially if the developer users custom triggers (REPLACE triggers a delete + insert if the record exists, while upsert just triggers an update).

It may even be useful to change or JSON tables to use upserts, but that may need a separate investigation on the implications.

As far as I know, the only advantage to using a REPLACE statement is to simplify the query (less duplication of column names), which is not relevant when we're generating it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants