Skip to content

Conversation

@mdevilliers
Copy link
Contributor

Looking as https://clickhouse.com/docs/operations/settings/settings some SETTINGS are defined as Bool with the value either being a 1 or a 0.
The clickhouse codebase and examples look like it is valid to define these as the value true or false as well as 1 or 0.
This PR adds the ability to parse both.

I am not too sure to this solution as the output will be harder to parse by an end user given that they will need to take care to accommodate either a NumberLiteral or a BoolLiteral. Maybe both cases should return the same one e.g.

var expr Expr
	switch {
	case p.matchTokenKind(TokenKindInt):
		number, err := p.parseNumber(p.Pos())
		if err != nil {
			return nil, err
		}
		expr = number
	...
	case p.matchKeyword(KeywordTrue), p.matchKeyword(KeywordFalse):
		// Handle TRUE/FALSE keywords as boolean literals
		lastToken := p.last()
		_ = p.lexer.consumeToken()
		l := &NumberLiteral{
			NumPos: lastToken.Pos,
			NumEnd: lastToken.Pos + 1,
			Literal:    "1",
			Base : 10,
		}
		if p.matchKeyword(KeywordFalse) {
			l.Literal = "0"
		}
		expr = l
	default:
		return nil, fmt.Errorf("unexpected token: %q, expected <number>, <bool> or <string>", p.last().String)
	}

Happy to change as you think?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 19433471793

Details

  • 25 of 43 (58.14%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 50.959%

Changes Missing Coverage Covered Lines Changed/Added Lines %
parser/parser_table.go 11 12 91.67%
parser/parse_system.go 8 12 66.67%
parser/ast_visitor.go 0 5 0.0%
parser/ast.go 6 14 42.86%
Files with Coverage Reduction New Missed Lines %
parser/ast_visitor.go 1 0.0%
Totals Coverage Status
Change from base Build 19430392194: 0.02%
Covered Lines: 7867
Relevant Lines: 15438

💛 - Coveralls

@git-hulk git-hulk changed the title feat: parse SETTING boolean values Add support of parsing SETTING boolean values Nov 18, 2025
@git-hulk
Copy link
Member

@mdevilliers Thanks for your PR. It looks pretty good to me.

I am not too sure to this solution as the output will be harder to parse by an end user given that they will need to take care to accommodate either a NumberLiteral or a BoolLiteral.

It would be better to keep the original boolean type, so I prefer to keep the current implementation.

@git-hulk git-hulk merged commit 8ca8a97 into AfterShip:master Nov 18, 2025
1 check passed
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