Skip to content

Conversation

swgillespie
Copy link

Compression codecs such as DoubleDelta do not require any additional positional parameters and infer defaults, such as CODEC(DoubleDelta). This commit doesn't continue parsing a codec if there's not a comma indicating additional parameters to the codec.

Compression codecs such as `DoubleDelta` do not require any additional positional parameters and infer defaults, such as `CODEC(DoubleDelta)`. This commit doesn't continue parsing a codec if there's not a comma indicating additional parameters to the codec.
@swgillespie
Copy link
Author

I see in this test that the formatting is likely wrong, e.g.

CREATE TABLE shark_attacks (timestamp DateTime CODEC(DoubleDelta, DoubleDelta));

I'm not sure what the maintainers would expect for the values of Name and Type would be for CODEC(DoubleDelta). I'm happy to make it match whatever it should be.

@git-hulk git-hulk self-requested a review September 30, 2025 07:51
@coveralls
Copy link

Pull Request Test Coverage Report for Build 18118488558

Details

  • 4 of 8 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 50.235%

Changes Missing Coverage Covered Lines Changed/Added Lines %
parser/parser_column.go 4 8 50.0%
Totals Coverage Status
Change from base Build 18089258850: 0.004%
Covered Lines: 7383
Relevant Lines: 14697

💛 - Coveralls

@git-hulk
Copy link
Member

I see in this test that the formatting is likely wrong, e.g.

CREATE TABLE shark_attacks (timestamp DateTime CODEC(DoubleDelta, DoubleDelta));

I'm not sure what the maintainers would expect for the values of Name and Type would be for CODEC(DoubleDelta). I'm happy to make it match whatever it should be.

Thanks for your improvement.

The correct format should be timestamp DateTime CODEC(DoubleDelta) instead of CODEC(DoubleDelta, DoubleDelta)). It's caused by using the same variant name: name when parsing codecType at parser_column.go#L1129, we can rename it to codeType. And skip the name if it's empty at ast.go#L4515.

@swgillespie
Copy link
Author

@git-hulk sounds good, will do.

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