Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(web-console): Add DEDUP support for Copy schema to clipboard #190

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

insmac
Copy link
Contributor

@insmac insmac commented Aug 17, 2023

Closes #184

Adding dedup configuration to table schema

If DEDUP UPSERT KEYS(key1, key2) has been used when creating a table, it should also be included in the generated DDL statement in Copy schema to clipboard.

Steps to test:

  1. Create a table that has deduplication enabled
CREATE TABLE TICKS (
    ts TIMESTAMP,
    tk SYMBOL,
    p DOUBLE
) TIMESTAMP(ts) PARTITION BY DAY WAL
DEDUP UPSERT KEYS(ts, tk);

INSERT INTO TICKS VALUES ('2023-07-14', 'QQQ', 78.56);
INSERT INTO TICKS VALUES ('2023-07-14', 'AAA', 78.56);
INSERT INTO TICKS VALUES ('2023-07-14', 'QQQ', 78.56); -- this is a dupe, won't be inserted
SELECT * FROM TICKS; -- should show 2 records
  1. Right-click on the table, select Copy schema to clipboard. The copied statement should include the DEDUP... statement from the first CREATE query above.

Adding dedup option to Create Table UI

This PR also adds the new "dedup" button in each column row, so that user might mark these as deduplication keys.

Disabled - incompatible column type or WAL not enabled:
CleanShot 2023-08-22 at 15 09 11@2x

Can be enabled - all good:
CleanShot 2023-08-22 at 15 09 19@2x

@insmac insmac added the web-console Issues relevant to "web-console" package label Aug 17, 2023
@insmac insmac changed the title chore(web-console): Add DEDUP support for Copy schema to clipboard [WIP] chore(web-console): Add DEDUP support for Copy schema to clipboard Aug 18, 2023
@insmac insmac changed the title [WIP] chore(web-console): Add DEDUP support for Copy schema to clipboard [WIP - Do not merge yet] chore(web-console): Add DEDUP support for Copy schema to clipboard Aug 18, 2023
@insmac insmac changed the title [WIP - Do not merge yet] chore(web-console): Add DEDUP support for Copy schema to clipboard chore(web-console): Add DEDUP support for Copy schema to clipboard Aug 22, 2023
@javier
Copy link

javier commented Aug 23, 2023

I've found a weird issue. If you create a table with dedup, then disable, it might still generate a DEDUP with empty keys (invalid). But not always?

Check this out

  1. create table
CREATE TABLE 'ilp_test2' (
  device_type SYMBOL capacity 256 CACHE,
  duration_ms LONG,
  lat DOUBLE,
  lon DOUBLE,
  measure1 LONG,
  timestamp TIMESTAMP,
  measure2 LONG,
  speed LONG
) timestamp (timestamp) PARTITION BY DAY WAL DEDUP UPSERT KEYS(timestamp, device_type);
  1. disable DEDUP
alter table ilp_test2 dedup disable;
  1. copy schema to clipboard and maybe see it fail
  2. drop table and try again?

@javier
Copy link

javier commented Aug 23, 2023

When using the web console to create table, I can create a table using DEDUP columns even if the designated timestamp is not part of the dedup. Designated timestamp must be selected if dedup is enabled. Not sure what the right action would be here:

  1. Leave it as is. User will get an error and will need to amend (maybe not too bad)
  2. If designated timestamp hasn't been selected, silently add it anyway to the UPSERT KEYS on generation
  3. Validate and don't allow users to submit unless designated ts has been selected as KEY if there is any other KEY already selected

@javier
Copy link

javier commented Aug 23, 2023

When using the web console, it properly says I cannot add a non valid type column as UPSERT key. The hover message even says that WAL must be enabled. But even when WAL is not enables, the interface still allows me to add UPSERT keys for symbols or ints and will happily generate the invalid SQL.

image

@insmac
Copy link
Contributor Author

insmac commented Aug 30, 2023

I've found a weird issue. If you create a table with dedup, then disable, it might still generate a DEDUP with empty keys (invalid). But not always?

Check this out

  1. create table
CREATE TABLE 'ilp_test2' (
  device_type SYMBOL capacity 256 CACHE,
  duration_ms LONG,
  lat DOUBLE,
  lon DOUBLE,
  measure1 LONG,
  timestamp TIMESTAMP,
  measure2 LONG,
  speed LONG
) timestamp (timestamp) PARTITION BY DAY WAL DEDUP UPSERT KEYS(timestamp, device_type);
  1. disable DEDUP
alter table ilp_test2 dedup disable;
  1. copy schema to clipboard and maybe see it fail
  2. drop table and try again?

@javier I've been able to replicate the above issue and provide a fix. Could you pull the branch and check again? Thanks! 🙂

@insmac
Copy link
Contributor Author

insmac commented Aug 30, 2023

When using the web console, it properly says I cannot add a non valid type column as UPSERT key. The hover message even says that WAL must be enabled. But even when WAL is not enables, the interface still allows me to add UPSERT keys for symbols or ints and will happily generate the invalid SQL.

image

@javier I have fixed the above issue, and added a check that even if user selects a dedup key first and disables WAL afterwards, the system will detect it and won't try to set UPSERT KEYS.

@insmac
Copy link
Contributor Author

insmac commented Aug 30, 2023

When using the web console to create table, I can create a table using DEDUP columns even if the designated timestamp is not part of the dedup. Designated timestamp must be selected if dedup is enabled. Not sure what the right action would be here:

Leave it as is. User will get an error and will need to amend (maybe not too bad)
If designated timestamp hasn't been selected, silently add it anyway to the UPSERT KEYS on generation
Validate and don't allow users to submit unless designated ts has been selected as KEY if there is any other KEY already selected

@javier I have decided to add the designated timestamp if user forgot to do it, instead of throwing an error.
This is because if there are other dedup keys present, I feel like he might just miss that it's required, and we can still use his settings (other keys) to proceed.

@javier
Copy link

javier commented Aug 31, 2023

I've found a weird issue. If you create a table with dedup, then disable, it might still generate a DEDUP with empty keys (invalid). But not always?
Check this out

  1. create table
CREATE TABLE 'ilp_test2' (
  device_type SYMBOL capacity 256 CACHE,
  duration_ms LONG,
  lat DOUBLE,
  lon DOUBLE,
  measure1 LONG,
  timestamp TIMESTAMP,
  measure2 LONG,
  speed LONG
) timestamp (timestamp) PARTITION BY DAY WAL DEDUP UPSERT KEYS(timestamp, device_type);
  1. disable DEDUP
alter table ilp_test2 dedup disable;
  1. copy schema to clipboard and maybe see it fail
  2. drop table and try again?

@javier I've been able to replicate the above issue and provide a fix. Could you pull the branch and check again? Thanks! 🙂

This works now. But I have the contrary effect. If I disable upsert on a table and then enable again, when I get the schema it comes with no DEDUP. If I alter a second time, then it is actually updated when I get the schema. Same if I change the schema on a different tab or via REST API. The get table schema gets the former version rather than the updated one.

@javier
Copy link

javier commented Aug 31, 2023

When using the web console, it properly says I cannot add a non valid type column as UPSERT key. The hover message even says that WAL must be enabled. But even when WAL is not enables, the interface still allows me to add UPSERT keys for symbols or ints and will happily generate the invalid SQL.
image

@javier I have fixed the above issue, and added a check that even if user selects a dedup key first and disables WAL afterwards, the system will detect it and won't try to set UPSERT KEYS.

tried it and seems to work fine :)

@javier
Copy link

javier commented Aug 31, 2023

Found a new one:

First try setting a table name and a table with both a designated timestamp and symbol (for example) and add both to the upsert keys

image

Now change the type from ts to SYMBOL
image

Invalid CREATE TABLE statement is generated

CREATE TABLE 'xxxx' (ts SYMBOL, sss SYMBOL) timestamp (ts) PARTITION BY DAY WAL DEDUP UPSERT KEYS(ts, sss);

This even works when I choose String or Binary as types. The UI will remove the UPSERT key check as it is not suported for those types, but still on create no errors and this is generated

CREATE TABLE 'xxxxxxxxx' (ts BINARY, sss SYMBOL) timestamp (ts) PARTITION BY HOUR WAL DEDUP UPSERT KEYS(ts, sss);

@javier
Copy link

javier commented Aug 31, 2023

When using the web console to create table, I can create a table using DEDUP columns even if the designated timestamp is not part of the dedup. Designated timestamp must be selected if dedup is enabled. Not sure what the right action would be here:

Leave it as is. User will get an error and will need to amend (maybe not too bad)
If designated timestamp hasn't been selected, silently add it anyway to the UPSERT KEYS on generation
Validate and don't allow users to submit unless designated ts has been selected as KEY if there is any other KEY already selected

@javier I have decided to add the designated timestamp if user forgot to do it, instead of throwing an error. This is because if there are other dedup keys present, I feel like he might just miss that it's required, and we can still use his settings (other keys) to proceed.

This works fine now

@insmac
Copy link
Contributor Author

insmac commented Sep 1, 2023

@javier Fixed the above issues:

  • When the designated timestamp is set and the column type is switched to something other than TIMESTAMP, it will clear it out in the background.
  • UPSERT KEYS statement should now be refreshed every time ALTER TABLE is executed.

Additionally:

  • DDL Schema is no longer added to the editor, because it was indeed confusing. Now, it is executed upon submitting the form and an appropriate notification (success/error) is dispatched and appears in the Notifications bar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
web-console Issues relevant to "web-console" package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DEDUP statement to Copy schema to clipboard feature
2 participants