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.

Sign up for GitHub

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

Jump to bottom

Parser cleanup and preparation for improved testing setup #3027

Merged
merged 6 commits into from
Oct 11, 2022
Merged

Parser cleanup and preparation for improved testing setup #3027

merged 6 commits into from
Oct 11, 2022

Conversation

JohanAhlen
Copy link
Contributor

This change contains the following improvements:

  • Improve the readability of the parser generator tool, switch to async/await and a major cleanup
  • Move jison files to a subfolder next to the parser being generated, making it easier to develop a parser when it's all in one place
  • Remove broken and unused new parser bootstrap tool
  • Remove generation of dynamic imports (sqlParserRepository.ts), as it's not always the case that the generated file will be the actual parser to use moving forward
  • Revert deletion of jison files (solr and global search) that went missing at some point
  • Regenerate all parsers to ensure up to date with jison version and grammar files

The main goal of this was to move the jison definitions to a subfolder next to the generated parser but while doing so I also decided that the parser generator needed some improvemnts. Having the jison next to the generated files helps with development and it will also enable us to reduce testing duplication (changes in an upcoming PR).

Tested by:

  • Generating all parsers as well as individual ones via CLI
  • Running existing unit tests
  • Manual sanity check in the editor

@JohanAhlen
[frontend] Remove the druid and elasticsearch parsers
9bbc952 
These were no different than the generic parser so removing to reduce duplication
@JohanAhlen
[frontend] Add solr and global search parsers back
9e4dad0 
These were removed by mistake at some point and this commit reverts that
@JohanAhlen
Copy link
Contributor Author

You can safely ignore the jison files, they're just moved around.

Main changes are in tools/jison

@JohanAhlen JohanAhlen force-pushed the jahlen_parser_cleanup branch from bdc0956 to 4df560d Compare September 29, 2022 14:20
bjornalm
bjornalm approved these changes Sep 29, 2022
View reviewed changes
Copy link
Collaborator

@bjornalm bjornalm left a comment

Choose a reason for hiding this comment

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

Excellent work,
just a questions and a request for additional clarification for stuff like

.replace(
          'loc: yyloc,',
          "loc: lexer.yylloc, ruleId: stack.slice(stack.length - 2, stack.length).join(''),"
        )

that is hard to grasp for parser mortals.

tools/jison/utils.js Outdated

import fs from 'fs';

export const listDir = async folder =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use the promise versions of fs.readFile, fs.readdir and fs.writeFile instead of creating your own async wrappers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I just copied the functions we had before, I'll switch to the promise versions.

tools/jison/parserDefinitions.js
sqlParser: autocomplete ? 'AUTOCOMPLETE' : 'SYNTAX',
parserName,
outputFolder,
afterParse: async contents =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add some comments to explain why and what we are replacing here? Or split this up into functions with descriptive names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Makes sense, I'll add some details.

@JohanAhlen
Copy link
Contributor Author

Excellent work, just a questions and a request for additional clarification for stuff like

.replace(
          'loc: yyloc,',
          "loc: lexer.yylloc, ruleId: stack.slice(stack.length - 2, stack.length).join(''),"
        )

that is hard to grasp for parser mortals.

True. In this particular case it's patching of jison which is sadly not maintained anymore (zaach/jison#356). I'll review and add some additional comments.

@JohanAhlen
[frontend] Improve autocomplete and syntax parser generation and stru…
2505f3f 
…cture

- Improve the readability of the generator tool, switch to async/await and a major cleanup
- Move jison files to a subfolder next to the parser being generated, making it easier to develop a parser when it's all in one place.
- Remove broken and unused new parser bootstrap tool
- Remove generation of dynamic imports (sqlParserRepository.ts), as it's not always the case that the generated file will be the actual parser to use moving forward
@JohanAhlen
[frontend] Regenerate all the parsers
3c26e17 
In some cases the generated parsers were out of sync with their jison definitions, likely caused by not generating after jison version bump.

For Hive, Impala and the generic parsers the only change is the introduction of jsdoc on the parse function.
@JohanAhlen JohanAhlen force-pushed the jahlen_parser_cleanup branch from 4df560d to 3c26e17 Compare September 30, 2022 08:59
@JohanAhlen JohanAhlen merged commit dbc7631 into master Oct 11, 2022
@JohanAhlen JohanAhlen deleted the jahlen_parser_cleanup branch October 11, 2022 11:45
agl29 pushed a commit that referenced this pull request Jul 24, 2023
@JohanAhlen @wing2fly
CDPD-40713: [frontend] Remove the druid and elasticsearch parsers (#3027
6010321 
)

These were no different than the generic parser so removing to reduce duplication

(cherry picked from commit 61507f0)
(cherry picked from commit 120fb86)
Change-Id: Ib4a5122e64480f755f8ab20ed9cf6bd3f454792a
(cherry picked from commit 673414c)
agl29 pushed a commit that referenced this pull request Jul 24, 2023
@JohanAhlen @wing2fly
CDPD-40713: [frontend] Add solr and global search parsers back (#3027)
309c432 
These were removed by mistake at some point and this commit reverts that

(cherry picked from commit b301075)
(cherry picked from commit da688b7)
Change-Id: Ice24d7d0790140b2f73d31d7bd75a4c79d6c5b02
(cherry picked from commit 0167bcf)
agl29 pushed a commit that referenced this pull request Jul 24, 2023
@JohanAhlen @wing2fly
CDPD-40713: [frontend] Improve autocomplete and syntax parser generat…
f0e378e 
…ion and structure (#3027)

- Improve the readability of the generator tool, switch to async/await and a major cleanup
- Move jison files to a subfolder next to the parser being generated, making it easier to develop a parser when it's all in one place.
- Remove broken and unused new parser bootstrap tool
- Remove generation of dynamic imports (sqlParserRepository.ts), as it's not always the case that the generated file will be the actual parser to use moving forward

(cherry picked from commit ba3ae3a)
(cherry picked from commit cc3fa3e)
Change-Id: I630c69f0eef5e958de0034b182e95021ddb8bbb8
(cherry picked from commit 16787e3)
agl29 pushed a commit that referenced this pull request Jul 24, 2023
@JohanAhlen @wing2fly
CDPD-40713: [frontend] Regenerate all the parsers (#3027)
cdecee4 
In some cases the generated parsers were out of sync with their jison definitions, likely caused by not generating after jison version bump.

For Hive, Impala and the generic parsers the only change is the introduction of jsdoc on the parse function.

(cherry picked from commit dbc7631)
(cherry picked from commit 7716ce0)
Change-Id: I7fb6831757377c2ac784ad1c0205f3cdf39400c4
(cherry picked from commit e5c4723)
@bjornalm bjornalm mentioned this pull request Feb 21, 2024
Closed
1 task
athithyaaselvam pushed a commit that referenced this pull request Mar 14, 2024
@JohanAhlen @wing2fly
CDPD-40713: [frontend] Remove the druid and elasticsearch parsers (#3027
673414c 
)

These were no different than the generic parser so removing to reduce duplication

(cherry picked from commit 61507f0)
(cherry picked from commit 120fb86)
Change-Id: Ib4a5122e64480f755f8ab20ed9cf6bd3f454792a
athithyaaselvam pushed a commit that referenced this pull request Mar 14, 2024
@JohanAhlen @wing2fly
CDPD-40713: [frontend] Add solr and global search parsers back (#3027)
0167bcf 
These were removed by mistake at some point and this commit reverts that

(cherry picked from commit b301075)
(cherry picked from commit da688b7)
Change-Id: Ice24d7d0790140b2f73d31d7bd75a4c79d6c5b02
athithyaaselvam pushed a commit that referenced this pull request Mar 14, 2024
@JohanAhlen @wing2fly
CDPD-40713: [frontend] Improve autocomplete and syntax parser generat…
16787e3 
…ion and structure (#3027)

- Improve the readability of the generator tool, switch to async/await and a major cleanup
- Move jison files to a subfolder next to the parser being generated, making it easier to develop a parser when it's all in one place.
- Remove broken and unused new parser bootstrap tool
- Remove generation of dynamic imports (sqlParserRepository.ts), as it's not always the case that the generated file will be the actual parser to use moving forward

(cherry picked from commit ba3ae3a)
(cherry picked from commit cc3fa3e)
Change-Id: I630c69f0eef5e958de0034b182e95021ddb8bbb8
athithyaaselvam pushed a commit that referenced this pull request Mar 14, 2024
@JohanAhlen @wing2fly
CDPD-40713: [frontend] Regenerate all the parsers (#3027)
e5c4723 
In some cases the generated parsers were out of sync with their jison definitions, likely caused by not generating after jison version bump.

For Hive, Impala and the generic parsers the only change is the introduction of jsdoc on the parse function.

(cherry picked from commit dbc7631)
(cherry picked from commit 7716ce0)
Change-Id: I7fb6831757377c2ac784ad1c0205f3cdf39400c4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjornalm bjornalm bjornalm approved these changes

@tabraiz12 tabraiz12 Awaiting requested review from tabraiz12

@agl29 agl29 Awaiting requested review from agl29

@Harshg999 Harshg999 Awaiting requested review from Harshg999

@nidhibhatg nidhibhatg Awaiting requested review from nidhibhatg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JohanAhlen @bjornalm