Skip to content

feat: expand xBase->SQL push-down coverage (\$ contains, LEFT)#108

Merged
Admnwk merged 1 commit into
FiveTechSoft:mainfrom
Admnwk:feat/sql-pushdown-coverage
Jun 26, 2026
Merged

feat: expand xBase->SQL push-down coverage (\$ contains, LEFT)#108
Admnwk merged 1 commit into
FiveTechSoft:mainfrom
Admnwk:feat/sql-pushdown-coverage

Conversation

@Admnwk

@Admnwk Admnwk commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #107. Widens the conservative try_emit_sql_where translator:

  • \$ (contains): 'needle' \$ haystack -> haystack LIKE '%needle%', for a literal needle with no LIKE wildcard. A field needle or an embedded % / _ / \ declines (returns nullopt), so the pushed predicate can never match different rows than the xBase original.
  • LEFT(s, n) -> SUBSTR(s, 1, n) (portable; SQLite has no native LEFT).

Still conservative — anything outside the modelled subset returns std::nullopt and the caller filters client-side. Unit tests: 11 cases / 116 assertions; full suite 834/834.

- xBase '\$' substring test: 'needle' \$ haystack -> haystack LIKE '%needle%',
  for a literal needle with no LIKE wildcard (field needle or embedded % _ \\
  declines, so matching can never change).
- LEFT(s, n) -> SUBSTR(s, 1, n) (portable; SQLite has no native LEFT).

try_emit_sql_where stays conservative: anything outside the modelled subset
still returns nullopt and the caller filters client-side. 11 cases /
116 assertions; full suite 834/834.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Admnwk Admnwk merged commit 3ecc8e8 into FiveTechSoft:main Jun 26, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for the xBase $ (substring) operator and the LEFT function to the SQL push-down generator, mapping them to SQL LIKE and SUBSTR respectively, along with corresponding unit tests. However, both features are currently missing implementations in the client-side row interpreter (src/engine/index_expr.cpp). This inconsistency can lead to silent query result corruption or incorrect evaluations if a query falls back to client-side filtering.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +102 to +116
if (i < s.size() && s[i] == '$') {
++i;
std::string rhs = emit_value();
if (!ok) return "";
if (lhs.size() >= 2 && lhs.front() == '\'' && lhs.back() == '\'') {
std::string inner = lhs.substr(1, lhs.size() - 2);
if (inner.find('%') == std::string::npos &&
inner.find('_') == std::string::npos &&
inner.find('\\') == std::string::npos) {
return rhs + " LIKE '%" + inner + "%'";
}
}
ok = false;
return "";
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While adding the $ (contains) operator to the SQL push-down is a great feature, $ is currently completely unsupported in the client-side row interpreter (eval_cmp in src/engine/index_expr.cpp). Because evaluate_index_expr_truthy does not enforce that the entire expression is consumed, any fallback to client-side filtering (e.g., if combined with an untranslatable predicate like RECNO() > 100) will cause the client-side parser to stop at $, treat the LHS literal as a bare truthy scalar, and silently ignore the rest of the expression (including the RHS and any subsequent .AND./.OR. conditions). For example, 'abc' $ NM .AND. QTY > 0 would evaluate to true for all rows client-side, completely ignoring both the substring check and the quantity check. To prevent silent query result corruption, please implement the $ operator in eval_cmp in src/engine/index_expr.cpp as well.

Comment on lines +239 to +240
if (fn == "LEFT" && args.size() >= 2)
return d.substr_fn + "(" + args[0] + ", 1, " + args[1] + ")";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While mapping LEFT to SUBSTR in the SQL push-down is correct, LEFT is currently not implemented in the client-side row interpreter (apply_scalar_fn in src/engine/index_expr.cpp). If a query containing LEFT falls back to client-side filtering (for example, if it is combined with an untranslatable function like RECNO()), LEFT will evaluate to an empty string because it falls into the unknown function fallback. This will lead to incorrect query results and inconsistency between server-side and client-side filtering. Please implement LEFT in apply_scalar_fn in src/engine/index_expr.cpp to ensure consistent evaluation.

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.

1 participant