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

feat: move AggregateFunctionsSuggestion to a separate interface, refactor general parser create tests #92

Merged
merged 15 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions CODE_CONVENTIONS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Code conventions

A set of rules all contributors should follow.
You can always link those rules in your PR without hesitation if you see that they are not followed.

## Testing

- Each test should only test a granular added piece of logic, and shouldn't test functionality of other's. E.g. if you are using OptionalIfNotExists, you shouldn't test if you get suggestions when writing '', 'IF ', 'IF NOT', just test if `IF NOT EXISTS` is suggesting, and it's enough.
- Each test file should contain at least:
- A test that checks a complete statement for errors
- A test that checks a complete statement locations (can be merged with the one above)
- Don't use `foo` or `bar` custom names, always use `test_{object}`, e.g. `SELECT * FROM test_table`, not `SELECT * FROM hehe_haha`. If you need multiple names, use `_{number}` suffix, e.g. `SELECT test_field, test_field_2 FROM test_table;`
- Write all the static tokens in UPPER_CASE, and all the custom variables in lower_case, e.g. `SELECT test_field`
- Always test your statements on errors, and if there's an unexpected error, just add `TODO: fix unhandled error` error
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I add something else, or is it complete?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's pretty good!

33 changes: 21 additions & 12 deletions src/parsing/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,17 @@ export abstract class Parser {
}

export interface ParseResult {
locations: StatementPart[];
locations: StatementPart[]; // TODO: figure our if it's optional
errors?: SyntaxError[];
suggestKeywords?: KeywordSuggestion[];
suggestTables?: {
prependFrom?: boolean;
prependQuestionMark?: boolean;
onlyTables?: boolean;
};
suggestTables?: TablesSuggestion;
suggestColumns?: ColumnSuggestion;
suggestAggregateFunctions?: {
tables: Table[],
};
suggestAggregateFunctions?: AggregateFunctionsSuggestion;
suggestAnalyticFunctions?: unknown;
suggestColRefKeywords?: unknown;
suggestColumnAliases?: ColumnAliasSuggestion[];
suggestCommonTableExpressions?: unknown;
suggestDatabases?: unknown;
suggestDatabases?: DatabasesSuggestion;
suggestFilters?: unknown;
suggestFunctions?: unknown;
suggestGroupBys?: unknown;
Expand All @@ -37,7 +31,7 @@ export interface ParseResult {
};

// Reasons for those fields are unknown
definitions?: [];
definitions?: []; // TODO: figure our if it's optional
lowerCase: boolean;
}

Expand Down Expand Up @@ -87,8 +81,23 @@ export type StatementPart =
qualified: boolean,
};

export interface TablesSuggestion {
prependFrom?: boolean; // TODO: figure our if it's optional
prependQuestionMark?: boolean; // TODO: figure our if it's optional
onlyTables?: boolean; // TODO: figure our if it's optional
onlyViews?: boolean; // TODO: figure our if it's optional
}

export interface DatabasesSuggestion {
appendDot?: boolean; // TODO: figure our if it's optional
}

export interface AggregateFunctionsSuggestion {
tables: Table[],
}

export interface ColumnSuggestion {
source?: string;
source?: string; // TODO: figure our if it's optional
tables: Table[];
}

Expand Down
2 changes: 1 addition & 1 deletion src/parsing/parsers/clickhouse/jison/structure.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"../../generic/jison/alter/alter_table.jison",
"../../generic/jison/alter/alter_view.jison",
"../../generic/jison/create/create_common.jison",
"../../generic/jison/create/create_database.jison",
"../../generic/jison/create/create_database_or_schema.jison",
"../../generic/jison/create/create_role.jison",
"create/create_table.jison",
"../../generic/jison/create/create_view.jison",
Expand Down
20 changes: 0 additions & 20 deletions src/parsing/parsers/generic/jison/alter/alter_common.test.json

This file was deleted.

14 changes: 14 additions & 0 deletions src/parsing/parsers/generic/jison/alter/alter_common.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import {
KeywordSuggestion,
parseGenericSql,
} from '../../../../index';
import {expect, test} from '@jest/globals';

test('should suggest ALTER', () => {
const parseResult = parseGenericSql('', '');

expect(parseResult.errors).toBeUndefined();

const suggestion: KeywordSuggestion = { value: 'ALTER', weight: -1 };
expect(parseResult.suggestKeywords).toContainEqual(suggestion);
})
1 change: 1 addition & 0 deletions src/parsing/parsers/generic/jison/alter/alter_table.jison
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ AlterTable
: AlterTableLeftSide PartitionSpec
;

// TODO: support AlterTableRightSide, PartitionSpec will not work for MySQL, PostgreSQL
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have ALTER TABLE table_name ADD column_name datatype; supported, sadly :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, noticed that..

AlterTable_EDIT
: AlterTableLeftSide_EDIT
| AlterTableLeftSide_EDIT PartitionSpec
Expand Down
41 changes: 0 additions & 41 deletions src/parsing/parsers/generic/jison/alter/alter_table.test.json

This file was deleted.

33 changes: 33 additions & 0 deletions src/parsing/parsers/generic/jison/alter/alter_table.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import {
DatabasesSuggestion,
KeywordSuggestion,
parseGenericSql, TablesSuggestion,
} from '../../../../index';
import {expect, test} from '@jest/globals';

test('should suggest altering table', () => {
const parseResult = parseGenericSql('ALTER ', '');

expect(parseResult.errors).toBeUndefined();

const suggestion: KeywordSuggestion = { value: 'TABLE', weight: -1 };
expect(parseResult.suggestKeywords).toContainEqual(suggestion);
})

test('should suggest tables to alter', () => {
const parseResult = parseGenericSql('ALTER TABLE ', '');

expect(parseResult.errors).toBeUndefined();

const tablesSuggestion: TablesSuggestion = {
onlyTables: true,
};
expect(parseResult.suggestTables).toEqual(tablesSuggestion);

const databasesSuggestion: DatabasesSuggestion = {
appendDot: true,
}
expect(parseResult.suggestDatabases).toEqual(databasesSuggestion);
})

// TODO: add full tests + locations test
81 changes: 0 additions & 81 deletions src/parsing/parsers/generic/jison/alter/alter_view.test.json

This file was deleted.

Loading
Loading