Skip to content

Compute TokenList.value dynamically (v2) - #710

Open
living180 wants to merge 3 commits into
andialbrecht:masterfrom
living180:dynamic_tokenlist_value_v2
Open

living180 wants to merge 3 commits into
andialbrecht:masterfrom
living180:dynamic_tokenlist_value_v2

Conversation

@living180

Copy link
Copy Markdown
Contributor

This PR supersedes #623. The meat of the PR is the same: fix the remaining portion of issue #621 by making TokenList.value a dynamically-computed property rather than an attribute. This avoids the quadratic runtime behavior that occurred due to recomputing TokenList.value each time TokenList.group_tokens() was called with extend=True.

The previous PR #623 had some rather awkward hacks related to stripping comments, but I found that I could avoid those by simply tweaking the comment stripping process to strip comments from a token list before stripping any sublists, making this PR much simpler.

@codecov

codecov Bot commented Mar 28, 2023

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.18%. Comparing base (111b35c) to head (14abdd6).
⚠️ Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #710      +/-   ##
==========================================
+ Coverage   97.13%   97.18%   +0.05%     
==========================================
  Files          31       31              
  Lines        3663     3804     +141     
  Branches      328      346      +18     
==========================================
+ Hits         3558     3697     +139     
- Misses         63       65       +2     
  Partials       42       42              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@living180
living180 force-pushed the dynamic_tokenlist_value_v2 branch 4 times, most recently from de63e50 to ff4f391 Compare March 30, 2023 10:11
@sdether

sdether commented Aug 11, 2023

Copy link
Copy Markdown

This is just the fix I need. Ran into some problems with parsing SQL with ~50k ID IN clauses, which with 0.4.4 takes a bit over 6 minutes to parse and with this patch only takes 6 seconds !!!

@rumbin

rumbin commented Nov 20, 2023

Copy link
Copy Markdown

any progress here?

@living180

Copy link
Copy Markdown
Contributor Author

Updated the PR to fix merge conflicts.

Avoid stripping T.Comment tokens contained within an sql.Comment before
stripping the sql.Comment itself.  Now an sql.Comment token will be
stripped first along with any contained T.Comment tokens.
@living180
living180 force-pushed the dynamic_tokenlist_value_v2 branch from 2da3e63 to 116b654 Compare September 15, 2026 11:48
Rename Token to TokenBase and make it a superclass for TokenList and a
new Token class.  Move some of the functionality of TokenBase into Token
and TokenList.  This will make it easier to maintain separate
functionality for Token versus TokenList.
@living180
living180 force-pushed the dynamic_tokenlist_value_v2 branch from 116b654 to 5239ee0 Compare September 15, 2026 11:53
The fact that a new value was being computed each time
TokenList.group_tokens() was called caused supralinear runtime when
token grouping was enabled.

Address by making TokenList.value a dynamically-computed property rather
than a static attribute.
@living180
living180 force-pushed the dynamic_tokenlist_value_v2 branch from 5239ee0 to 14abdd6 Compare September 15, 2026 12:00
@living180

Copy link
Copy Markdown
Contributor Author

Updated to fix merge conflicts and make tests pass again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants