Sqlite loader #67

rhazn · 2022-12-02T19:42:58Z

Summary

This PR adds support for a SQLiteLoader.

Known issues

If the order of attributes in the SQLiteLoader is not fixed, the following errors are shown when running npm run generate. I noticed the order of attributes in the CSVFileExtractor is also fixed. I am not sure how to fix this, maybe we can spin that out into an extra task.

Error: Parser definition errors detected:
-------------------------------
../..:6 - Ambiguous Alternatives Detected: <3 ,4> in <OR1> inside <BlockType> Rule,
<> may appears as a prefix path in all these alternatives.
See: https://chevrotain.io/docs/guide/resolving_grammar_errors.html#AMBIGUOUS_ALTERNATIVES
For Further details.
-------------------------------
../..:6 - Ambiguous alternatives: <3 ,4> due to common lookahead prefix
in <OR1> inside <BlockType> Rule,
<> may appears as a prefix path in all these alternatives.
See: https://chevrotain.io/docs/guide/resolving_grammar_errors.html#COMMON_PREFIX
For Further details.
-------------------------------
../..:6 - Ambiguous alternatives: <3 ,4> due to common lookahead prefix
in <OR1> inside <BlockType> Rule,
<> may appears as a prefix path in all these alternatives.
See: https://chevrotain.io/docs/guide/resolving_grammar_errors.html#COMMON_PREFIX
For Further details.
-------------------------------
../..:6 - Ambiguous alternatives: <3 ,4> due to common lookahead prefix
in <OR1> inside <BlockType> Rule,
<> may appears as a prefix path in all these alternatives.
See: https://chevrotain.io/docs/guide/resolving_grammar_errors.html#COMMON_PREFIX
For Further details.
-------------------------------
../..:6 - Ambiguous alternatives: <3 ,4> due to common lookahead prefix
in <OR1> inside <BlockType> Rule,
<> may appears as a prefix path in all these alternatives.
See: https://chevrotain.io/docs/guide/resolving_grammar_errors.html#COMMON_PREFIX
For Further details.
-------------------------------
../..:6 - Ambiguous alternatives: <3 ,4> due to common lookahead prefix
in <OR1> inside <BlockType> Rule,
<> may appears as a prefix path in all these alternatives.
See: https://chevrotain.io/docs/guide/resolving_grammar_errors.html#COMMON_PREFIX
For Further details.
-------------------------------
../..:6 - Ambiguous alternatives: <3 ,4> due to common lookahead prefix
in <OR1> inside <BlockType> Rule,
<> may appears as a prefix path in all these alternatives.
See: https://chevrotain.io/docs/guide/resolving_grammar_errors.html#COMMON_PREFIX
For Further details.

Screenshot

SQLiteLoader can load data into a local SQLite database file. Currently the table name is called "Todo" and not set by the block due to errors with the grammar.

Adds a table attribute to SQLiteLoader so that the table name can be defined. The ordering of attributes is intentially fixed as introducing unordered attributes with '&' seems to lead to a lot of COMMON_PREFIX and AMBIGUOUS_ALTERNATIVES errors with PostgresLoader.

With the addition of surrounding delimiters around column names with a recent patch, the EMPTY fallback is no longer necessary as empty column names actually work.

Cars example now uses SQLite instead of Postgres. This means it also does not need to start a local Postgres DB anymore.

felix-oq · 2022-12-02T22:16:08Z

felix-oq approved these changes

Dec 5, 2022

View reviewed changes

Awesome that we support SQLite as a sink now. This really helps users to get started with less setup. I'm also glad to see that you were able to add the new loader block to the grammar by yourself :)

I left some comments but LGTM otherwise.

An additional comment:
We should add .db and .sqlite files to gitignore, so they are not committed accidentally.

apps/interpreter/src/executors/postgres-loader-executor.ts

See #61 regarding the problem with the attribute ordering.

@@ @@ -59,7 +59,7 @@ export class PostgresLoaderExecutor extends BlockExecutor< @@
                   const columnTypeVisitor = new PostgresColumnTypeVisitor();
                   const columnPostgresStatements = input.columnNames
-                    .map((columnName) => columnName || 'EMPTYNAME')
+                    .map((columnName) => columnName)

felix-oq

Dec 5, 2022

Suggested change

.map((columnName) => columnName)

Can be omitted as it does not do anything.

felix-oq · 2022-12-05T07:47:47Z

apps/interpreter/src/executors/postgres-loader-executor.ts

@@ -88,7 +88,7 @@ export class PostgresLoaderExecutor extends BlockExecutor<
      .join(',');

    return `INSERT INTO "${tableName}" (${input.columnNames
-      .map((columnName) => columnName || 'EMPTYNAME')
+      .map((columnName) => columnName)


Suggested change

.map((columnName) => columnName)

Again, can be omitted.

felix-oq · 2022-12-05T07:52:04Z

example/cars.jv

 		table: "Cars";
+		file: "./cars.db";


You could add a custom validator for checking the file ending and e.g. show a warning if it is not .sqlite or .db.

I am not actually sure there has to be a specific file extension, I don't know sqlite enough :D. In any case I'll have to leave that as a future exercise for me because it is a new system I have not worked with yet (the custom validations). I'll get to it though.

We can also pair program this if you want to :)

True, good idea. I'll have to keep it in mind for the future though, not sure I can get to it this week. But I'll ping you :).

felix-oq · 2022-12-05T07:55:21Z

libs/language-server/src/grammar/attributes.langium

+
+TableAttribute returns StringAttribute:
+  'table' StringAttributeFragment;


Maybe move this into the blocks folder in a file called shared-attributes.langium? That way attributes.langium file does not contain any block-specific attributes.

felix-oq · 2022-12-05T08:08:46Z

apps/interpreter/src/visitors/SQLColumnTypeVisitor.ts

@@ -0,0 +1,16 @@
+import { DataTypeVisitor } from '@jayvee/language-server';
+
+export class SQLColumnTypeVisitor extends DataTypeVisitor<string> {


We could omit the subclasses PostgresColumnTypeVisitor and SQLiteColumnTypeVisitor because they don't add any behavior. Instead we could go with SQLColumnTypeVisitor and add subclasses in the future when required.

Same for SQLValueRepresentationVisitor, PostgresValueRepresentationVisitor and SQLiteValueRepresentationVisitor.

felix-oq · 2022-12-05T08:17:14Z

apps/interpreter/src/executors/sqlite-loader-executor.ts

+  private buildCreateTableStatement(tableName: string, input: Table): string {
+    const columnTypeVisitor = new SQLiteColumnTypeVisitor();
+
+    const columnStatements = input.columnNames
+      .map((columnName) => columnName)
+      .map((columnName) => `"${columnName}"`)
+      .map((name, index) => {
+        return `${name} ${(
+          input.columnTypes[index] as AbstractDataType
+        ).acceptVisitor(columnTypeVisitor)}`;
+      });
+
+    return `CREATE TABLE IF NOT EXISTS "${tableName}" (${columnStatements.join(
+      ',',
+    )});`;
+  }
+
+  private buildInsertValuesStatement(tableName: string, input: Table): string {
+    const valueRepresenationVisitor = new SQLiteValueRepresentationVisitor();
+
+    const valueRepresentationFormatters = input.columnTypes.map((type) => {
+      return type?.acceptVisitor(valueRepresenationVisitor);
+    });
+
+    const valuesStatement = input.data
+      .map((row) => {
+        return `(${row
+          .map((value, index) => valueRepresentationFormatters[index]?.(value))
+          .join(',')})`;
+      })
+      .join(',');
+
+    return `INSERT INTO "${tableName}" (${input.columnNames
+      .map((columnName) => columnName)
+      .map((columnName) => `"${columnName}"`)
+      .join(',')}) VALUES ${valuesStatement}`;
+  }


All of this is pretty much duplicate code (compared to postgres-loader-executor). Consider a refactoring.

felix-oq · 2022-12-05T08:32:05Z

apps/interpreter/src/executors/sqlite-loader-executor.ts

+        }),
+      );
+    } finally {
+      db && db.close();


Suggested change

db && db.close();

db?.close();

rhazn added 4 commits December 2, 2022 20:01

Added new SQLiteLoader Blocktype

3f58d50

SQLiteLoader can load data into a local SQLite database file. Currently the table name is called "Todo" and not set by the block due to errors with the grammar.

Removed accidentially committed cars.db

1b43ecf

Removed EMPTY fallback for column names

5a30663

With the addition of surrounding delimiters around column names with a recent patch, the EMPTY fallback is no longer necessary as empty column names actually work.

rhazn requested a review from felix-oq December 2, 2022 19:43

rhazn added 2 commits December 2, 2022 20:46

Update readme with new cars example

5fc2a36

Cars example now uses SQLite instead of Postgres. This means it also does not need to start a local Postgres DB anymore.

Fixed linting errors in readme

7bf32fd

Refactored according to review feedback

4af26e3

rhazn merged commit 825e2d4 into main Dec 5, 2022

rhazn deleted the sqlite-loader branch December 5, 2022 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Sqlite loader #67

Sqlite loader #67

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		TableAttribute returns StringAttribute:
		'table' StringAttributeFragment;

		@@ -0,0 +1,16 @@
		import { DataTypeVisitor } from '@jayvee/language-server';

		export class SQLColumnTypeVisitor extends DataTypeVisitor<string> {

Sqlite loader #67

Sqlite loader #67

Uh oh!

Conversation

Summary

Known issues

Screenshot

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!