Skip to content

fix(core-spec): escape || so it renders in expression language tables - #575

Merged
jbonofre merged 1 commit into
apache:mainfrom
christianeu-db:fix-concat-operator-rendering
Oct 13, 2026
Merged

jbonofre merged 1 commit into
apache:mainfrom
christianeu-db:fix-concat-operator-rendering

Conversation

@christianeu-db

@christianeu-db christianeu-db commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The || string concatenation operator did not render in two tables in
core-spec/expression_language.md. In GitHub-Flavored Markdown, an unescaped
| is a table column delimiter even inside a code span, so each || produced
extra empty columns in its row instead of displaying the operator.

This change escapes the pipe characters as \|\|, matching the existing
convention in converters/cube/README.md. It covers both affected tables:

  • String Manipulation (REQUIRED) — the || operator row.
  • Common Dialect Variations — the PostgreSQL a || b cell.

The ThoughtSpot converter parses this spec file to assert its catalog covers
every spec construct (converters/thoughtspot/.../expressions/test_catalog_covers_the_spec.py),
and that parser was built around the unescaped ||. This PR teaches its
_clean() helper to undo the \| escape so the extracted construct name stays
str1 || str2. Without that one-line parser change, the escaped source leaks
backslashes and the coverage test fails.

This is not a breaking change. The specification's meaning is unchanged —
|| was and remains a supported concatenation operator; only its rendering is
fixed. The included parser update keeps the ThoughtSpot converter's test suite
green (uv run pytest → 1015 passed).

Related Issues

Reported by @Haoranli503 in #547 (comment).

Checklist

Specification

  • Spec changes are included in core-spec/ and follow the existing structure
  • Spec changes have been discussed on the mailing list or in a linked issue (thread)
  • Breaking changes to the spec are clearly called out in the summary

Converters

  • Converter logic in converters/ is updated to reflect spec or ontology changes
  • New converters include tests under the converter's test directory

Tests

  • All existing tests pass (pytest / CI green)

Remaining checklist sections (Ontology, Validation, Documentation, Examples,
Compliance) are not applicable: this PR escapes two Markdown table cells and
adjusts one parser helper, adding no spec constructs, new files, or
dependencies.

AI disclosure

Per the ASF Generative Tooling Guidance, this contribution was prepared with AI
assistance. I have reviewed and verified every change.

In GitHub-Flavored Markdown an unescaped `|` acts as a table column
delimiter even inside a code span, so the `||` concatenation operator in
the String Manipulation and Common Dialect Variations tables spawned
phantom columns instead of rendering. Escape the pipes as `\|\|`, matching
the existing convention in converters/cube/README.md.

The thoughtspot converter parses this spec file to assert its catalog
covers every construct, and that parser was built around the unescaped
`||`. Teach its _clean() helper to undo the `\|` escape so the extracted
construct name stays `str1 || str2`; otherwise the escaped source leaks
backslashes and test_catalog_covers_the_spec fails.

Reported by @Haoranli503.

Co-authored-by: Isaac <no-reply@databricks.com>
@christianeu-db christianeu-db changed the title docs(core-spec): escape || so it renders in expression language tables fix(core-spec): escape || so it renders in expression language tables Oct 12, 2026
@christianeu-db
christianeu-db marked this pull request as ready for review October 12, 2026 20:38
@jbonofre
jbonofre self-requested a review October 13, 2026 09:27

@jbonofre jbonofre left a comment

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.

LGTM.

Escaping || as \|\| in the two expression_language.md table rows keeps the tables rendering correctly, and unescaping \| in the Thoughspot catalog parser's _clean restores the literal ||. I checked that _split_table_row ignores pipes inside backtick spans, so the escaped cells stay in one cell. The Thoughtspot catalog and its spec-coverage test are the only code that reads this file.

Nit: _clean now unescapes \| in every cell, not just the || rows. That's harmless today, but a future cell that needs a literal backslash-pipe would be affected. You could limit the unescape to the operator cells, or just keep it in mind.

@jbonofre
jbonofre merged commit 6ff8098 into apache:main Oct 13, 2026
81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants