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

problem parsing check constraint table definition #1454

Closed
eyeteajay opened this issue Jun 29, 2018 · 10 comments
Closed

problem parsing check constraint table definition #1454

eyeteajay opened this issue Jun 29, 2018 · 10 comments
Assignees

Comments

@eyeteajay
Copy link

@eyeteajay eyeteajay commented Jun 29, 2018

Under the table definition, there is a warning that says: "There is something wrong with this table definition that our parser doesn't fully understand." Also, the table cannot be expanded in the "database structure panel". However, the program seems to work and read the rows anyway.

Details for the issue

The problem is with ck_AssetTreeAssoc_depth, because the table definition can be parsed fine without it. My guess would be the nested parenthesis? The problem DDL is:

CREATE TABLE "AssetTreeAssoc" (
	uuid BINARY(16) NOT NULL, 
	cdt DATETIME DEFAULT (DATETIME('NOW')) NOT NULL, 
	mdt DATETIME DEFAULT (DATETIME('NOW')) NOT NULL, 
	"passetID" INTEGER NOT NULL, 
	"cassetID" INTEGER NOT NULL, 
	depth INTEGER DEFAULT 0 NOT NULL, 
	id INTEGER NOT NULL, 
	PRIMARY KEY (id), 
	CONSTRAINT "ck_AssetTreeAssoc_depth" CHECK ((passetID <> cassetID and depth > 1) or depth == 0),
	CONSTRAINT "uq_AssetTreeAssoc" UNIQUE ("cassetID", depth), 
	FOREIGN KEY("passetID") REFERENCES "AssetTree" (id) ON DELETE CASCADE, 
	FOREIGN KEY("cassetID") REFERENCES "AssetTree" (id) ON DELETE CASCADE
)

Useful extra information

The info below often helps, please fill it out if you're able to. :)

What operating system are you using?

  • Windows: ( version: ___ )
  • Linux: ( distro: ___ )
  • Mac OS: ( _version: 10.11.6 )
  • Other: ___

What is your DB4S version?

  • 3.10.1
  • 3.10.0
  • 3.9.1
  • Other: ___

Did you also

@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Jul 10, 2018

Thanks for the report and the detailed analysis, @eyeteajay! This is a problem in our grammar parser. We detect these sort of issues (which is why you get that warning) and fall back to a simpler approach. That's why you can still browse the table. But for other features and especially for editing the table there just isn't enough information this way.

Anyway, I'll have a closer look at the problem now 😄

@MKleusberg MKleusberg self-assigned this Jul 10, 2018
MKleusberg added a commit that referenced this issue Jul 10, 2018
This fixes parsing of expressions of the form '(x) AND/OR y' and similar
types. It also fixes expression of the '(x)' type and of the '(x op y
AND z op w)' type.

See issue #1454.
@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Jul 10, 2018

I believe the issue is fixed now 😄 Can you try tomorrow's nightly build and check if it's working for you, @eyeteajay? It's also worth checking if you have issues with any other tables.

@parno
Copy link

@parno parno commented Jul 30, 2018

FWIW, I'm seeing a similar issue with this (simplified) table definition:

CREATE TABLE "Transactions" (
	"stock_id"	INTEGER CHECK(
      (stock_id is null AND shares is null)
      OR
      (stock_id is not null AND shares is not null)),
	"shares"	INTEGER
);

I was able to create it via the UI, but the parser seems to struggle with it, even with today's (7/29) nightly build. I'm running on Mac OS X, 10.13.5.

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Oct 8, 2018

I tested with the queries in this issue. The first one couldn't be parsed until I removed the DEFAULT clauses:

CREATE TABLE "AssetTreeAssoc4" (
	uuid BINARY(16) NOT NULL, 
	cdt DATETIME, -- DEFAULT (DATETIME('NOW')) NOT NULL, 
	mdt DATETIME, -- DEFAULT (DATETIME('NOW')) NOT NULL, 
	"passetID" INTEGER NOT NULL, 
	"cassetID" INTEGER NOT NULL, 
	depth INTEGER DEFAULT 0 NOT NULL, 
	id INTEGER NOT NULL, 
	PRIMARY KEY (id), 
	CONSTRAINT "ck_AssetTreeAssoc_depth" CHECK ((passetID <> cassetID and depth > 1) or depth == 0),
	CONSTRAINT "uq_AssetTreeAssoc" UNIQUE ("cassetID", depth), 
	FOREIGN KEY("passetID") REFERENCES "AssetTree" (id) ON DELETE CASCADE, 
	FOREIGN KEY("cassetID") REFERENCES "AssetTree" (id) ON DELETE CASCADE
)

The second one has problems too. It worked when one side of the OR condition is entirely removed:

CREATE TABLE "Transactions3" (
	"stock_id"	INTEGER CHECK(
      stock_id is null AND shares is null),
--      OR
--      (stock_id is not null AND shares is not null)),
	"shares"	INTEGER
);

Removing the internal parenthesis is also parseable (of course, I'm not proposing this as a workaround):

CREATE TABLE "Transactions4" (
	"stock_id"	INTEGER CHECK(
      stock_id is null AND shares is null
      OR
      stock_id is not null AND shares is not null),
	"shares"	INTEGER
);
@moll
Copy link

@moll moll commented Dec 1, 2018

Also seeing the same here with v3.11.0-beta1 and the following schema:

CREATE TABLE initiatives (
  uuid STRING PRIMARY KEY NOT NULL,
  mailchimp_interest_id STRING NOT NULL UNIQUE,
  notes TEXT NOT NULL DEFAULT "",

  CONSTRAINT initiatives_uuid_length
  CHECK (length(uuid) == 36),

  CONSTRAINT initiatives_mailchimp_interest_id
  CHECK (length(mailchimp_interest_id) > 0)
);
@Unchqua
Copy link

@Unchqua Unchqua commented Mar 31, 2019

Having this issue too. My DDL text is:

CREATE TABLE word ( id INTEGER PRIMARY KEY AUTOINCREMENT, text TEXT NOT NULL, source INTEGER NOT NULL CHECK(CASE WHEN source=1 OR source=2 THEN NULL ELSE 0 END) );

I create this table manually (executing sqlite3.exe in Windows and feeding it with DDL) and in DB4S UI, and both ways DB4S can't parse this table's structure after. Note that I have no parenthesis in CHECK clause.
Using latest build 3.11.1 (SQLite Version 3.27.1) on Windows.

@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Sep 15, 2019

@moll @Unchqua We have just rewritten our parser to make it work much better with expressions like yours. Are you interested in giving the next nightly build (available here in about 24 hours) a try and see if it fixes your problem? 😄

@Unchqua
Copy link

@Unchqua Unchqua commented Sep 16, 2019

This seems like fixes that bug! Also, previous version 3.11.2 hasn't this issue either.
Latest nightly 3.11.99 makes it a bit better by adding proper spaces in text representation of a DDL:
3.11.2:
"source" INTEGER NOT NULL CHECK(CASEWHENsource IN (1,2,3,4,5)THENNULLELSE0END)
3.11.99:
"source" INTEGER NOT NULL CHECK(CASE WHEN "source" IN (1, 2, 3, 4, 5) THEN NULL ELSE 0 END)
Also, I love that button "Reset the order of rows to the default", it was really inconvenient to accidently set the order and then to be unable to drop that. Thanks!
Also, in both 3.11.1 and 3.11.2 I saw sometimes that text tabs in Execute SQL tab lost their focuses when you switch between Database Structure, Browse Data and Execute SQL tabs; now they aren't.
Still, should we consider this 3.11.99 a beta, with all the cautions for data manipulations on our databases? And wait for production-ready 3.12.0?

@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Sep 19, 2019

@Unchqua I'm glad it's working for you now and that you like the other changes as well 😄 It's hard to say how stable the nightly builds are. Looking at the stats though, about 200 people seem to use the nightly build on a regular basis. So it is actually somewhat tested. Personally I would say: wait another day and get tomorrow's nightly build which fixes a regression I introduced accidentally a while ago, and maybe keep an eye on the SQL preview when/if you are using the Edit Table dialog. But other than that you should be fine to use the nightlies until the 3.12.0 release is ready 😄

@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Feb 12, 2020

I'm closing this issue since the problem with the parser has been solved some time ago.
@Unchqua We're starting the release process now, so the latest nightly builds aren't too far away from what will be released soon. This means it should be pretty safe to use them 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants