Skip to content

Remove regex usage from Identifier validation#11548

Merged
findepi merged 1 commit intotrinodb:masterfrom
wendigo:serafin/simplify-identifier-validation
Mar 22, 2022
Merged

Remove regex usage from Identifier validation#11548
findepi merged 1 commit intotrinodb:masterfrom
wendigo:serafin/simplify-identifier-validation

Conversation

@wendigo
Copy link
Copy Markdown
Contributor

@wendigo wendigo commented Mar 17, 2022

Description

JMH results: https://jmh.morethan.io/?gists=d2ceb76e826da7295d86efce0088a550,02f65f59e650539dad816fa36c8805f2

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Core engine.

How would you describe this change to a non-technical end user or system administrator?

Worse -> better (tm @skrzypo987)

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Mar 17, 2022
@wendigo wendigo requested review from findepi and hashhar March 17, 2022 17:34
@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 17, 2022

Screenshot 2022-03-17 at 15 13 10

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 18, 2022

build isn't green

@wendigo wendigo force-pushed the serafin/simplify-identifier-validation branch from 45099ae to f27801f Compare March 21, 2022 10:30
@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 21, 2022

@findepi checkstyle. Fixed, PTAL now

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: .substring is copying operation. can we do better?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Of course :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Better now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the "TAIL" name no longer applies

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As always, you are right :)

@hashhar hashhar removed their request for review March 22, 2022 07:43
@wendigo wendigo force-pushed the serafin/simplify-identifier-validation branch from bf3bad0 to 42a1dfc Compare March 22, 2022 09:46
This improves query planner performance.
@findepi findepi force-pushed the serafin/simplify-identifier-validation branch from 42a1dfc to aa49c82 Compare March 22, 2022 10:20
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 22, 2022

Added some rationale to cmt msg

@findepi findepi merged commit 8b502a9 into trinodb:master Mar 22, 2022
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Mar 22, 2022
@wendigo wendigo deleted the serafin/simplify-identifier-validation branch March 22, 2022 16:50
@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 22, 2022

Thx @findepi !

@github-actions github-actions bot added this to the 375 milestone Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

2 participants