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

fix(SqliteDriver.js): Fixed table schema parsing #289

Merged
merged 2 commits into from
Dec 14, 2019
Merged

fix(SqliteDriver.js): Fixed table schema parsing #289

merged 2 commits into from
Dec 14, 2019

Conversation

philippefutureboy
Copy link
Contributor

@philippefutureboy philippefutureboy commented Dec 12, 2019

1- .match on line 52 returned null => fixed by removing the EOL character
2- The describe table returns the name of the field between square brackets => fixed by removing the square brackets (see https://www.sqlitetutorial.net/sqlite-tutorial/sqlite-describe-table/)

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves

(self contained, did not create an issue beforehand)

#1 .match on line 52 returned null => fixed by removing the EOL character
#2 The describe table returns the name of the field between square brackets => fixed by removing the square brackets
@paveltiunov
Copy link
Member

paveltiunov commented Dec 13, 2019

@philippefutureboy Hey Philippe! Thanks for contributing this! I believe square brackets are optional. Should we support both cases: with and without square brackets?

@philippefutureboy
Copy link
Contributor Author

@paveltiunov Hey Pavel :)
I didn't know that - we did not add any special parameters to the config object, and the db is a test db found online – https://www.dropbox.com/s/a2wax843eniq12g/flights.db?dl=0
Sure thing, if it is optional I can refactor that as a regex and match both cases.

Let me know :)

@paveltiunov
Copy link
Member

@philippefutureboy Yeah. Please do. [ and ] are just escape symbols and those are completely optional. Those also include " and ```: https://www.sqlite.org/lang_keywords.html.

Fix now supports all escape symbols and absence of escape symbol.
+ Refactored the `tables.map.reduce` to `tables.reduce` instead.
@philippefutureboy
Copy link
Contributor Author

@paveltiunov Done!

To test, simply use the test data below:

var tables = JSON.parse('[{"name":"airlines","sql":"\nCREATE TABLE airlines (\n[index] INTEGER,\n  [id] TEXT,\n  [name] TEXT,\n  [alias] TEXT,\n  [iata] TEXT,\n  [icao] TEXT,\n  [callsign] TEXT,\n  [country] TEXT,\n  [active] TEXT\n)\n"},{"name":"airlines2","sql":"\nCREATE TABLE airlines2 (\n`index` INTEGER,\n  `id` TEXT,\n  `name` TEXT,\n  `alias` TEXT,\n  `iata` TEXT,\n  `icao` TEXT,\n  `callsign` TEXT,\n  `country` TEXT,\n  `active` TEXT\n)\n"},{"name":"airlines3","sql":"\nCREATE TABLE airlines3 (\n\"index\" INTEGER,\n  \"id\" TEXT,\n  \"name\" TEXT,\n  \"alias\" TEXT,\n  \"iata\" TEXT,\n  \"icao\" TEXT,\n  \"callsign\" TEXT,\n  \"country\" TEXT,\n  \"active\" TEXT\n)\n"},{"name":"airlines4","sql":"\nCREATE TABLE airlines4 (\nindex INTEGER,\n  id TEXT,\n  name TEXT,\n  alias TEXT,\n  iata TEXT,\n  icao TEXT,\n  callsign TEXT,\n  country TEXT,\n  active TEXT\n)\n"}]'

And run against the code at lines 49-65 :)

@paveltiunov paveltiunov merged commit 42026fb into cube-js:master Dec 14, 2019
@paveltiunov
Copy link
Member

@philippefutureboy Looks great! Thanks for contributing this! Really appreciate it!

@philippefutureboy
Copy link
Contributor Author

Always a pleasure to contribute :)

@rpaik rpaik added the pr:community Contribution from Cube.js community members. label Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants