Skip to content

Normalize index identifiers on construction#142

Closed
crtschin wants to merge 1 commit into
masterfrom
dev-crtschin-legacy-checks
Closed

Normalize index identifiers on construction#142
crtschin wants to merge 1 commit into
masterfrom
dev-crtschin-legacy-checks

Conversation

@crtschin
Copy link
Copy Markdown
Collaborator

My changes in #136 didn't account for legacy indices that used a workaround to get columns with non-reserved keywords by explicitly quoting them in the includes. These indices historically got generated names with the " converted into $.

This PR changes it so index column identifiers are stripped of outer quotes at construction. Indexes created by older versions still validate via a reconstructed legacy name candidate.

Index column identifiers are stripped of outer quotes at construction.
Indexes created by older versions still validate via a reconstructed
legacy name candidate.
@crtschin crtschin force-pushed the dev-crtschin-legacy-checks branch from 3f6bb35 to 37c70e4 Compare May 22, 2026 11:48
@crtschin crtschin requested a review from arybczak May 22, 2026 12:45
@arybczak
Copy link
Copy Markdown
Collaborator

arybczak commented May 22, 2026

Is it actually the case that #136 was unnecessary since putting select columns in parentheses was possible before?

@crtschin
Copy link
Copy Markdown
Collaborator Author

Is it actually the case that #136 was unnecessary since putting select columns in parentheses was possible before?

If you mean double quotes, then yes. Quotes automatically caused it to fallthrough to this logic, making the index valid.

I guess this was more of a documentation problem, but now that #136 is running in prod we'd need to rename the index if we want to revert it. @marinelli, can you verify if you started depending on this functionality, i.e. did you already create an index on an unquoted timestamp?

@marinelli
Copy link
Copy Markdown
Contributor

Is it actually the case that #136 was unnecessary since putting select columns in parentheses was possible before?

If you mean double quotes, then yes. Quotes automatically caused it to fallthrough to this logic, making the index valid.

I guess this was more of a documentation problem, but now that #136 is running in prod we'd need to rename the index if we want to revert it. @marinelli, can you verify if you started depending on this functionality, i.e. did you already create an index on an unquoted timestamp?

I think we did that.

@arybczak
Copy link
Copy Markdown
Collaborator

I'd prefer we revert this change and fix a few stray indexes in journey rather than pile up more code for workarounds and special cases 🤔

@crtschin
Copy link
Copy Markdown
Collaborator Author

I'd prefer we revert this change and fix a few stray indexes in journey rather than pile up more code for workarounds and special cases 🤔

Yeah, makes sense. I'll close this and revert #136 on master and add a documentation note instead somewhere.

@crtschin crtschin closed this May 26, 2026
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