Skip to content

common : honor case_sensitive argument in jinja sort and dictsort - #24971

Closed
dnislno wants to merge 1 commit into
ggml-org:masterfrom
dnislno:fix/jinja-sort-case-sensitive
Closed

common : honor case_sensitive argument in jinja sort and dictsort#24971
dnislno wants to merge 1 commit into
ggml-org:masterfrom
dnislno:fix/jinja-sort-case-sensitive

Conversation

@dnislno

@dnislno dnislno commented Jun 24, 2026

Copy link
Copy Markdown

Overview

Fix the case_sensitive keyword argument being silently ignored in both the sort and dictsort Jinja filters in common/jinja/value.cpp.

Both filters read case_sensitive from kwargs but the line that applies it was commented out with a FIXME: sorting is currently always case sensitive marker. As a result, {{ items | sort(case_sensitive=false) }} and {{ obj | dictsort(case_sensitive=false) }} silently perform a case-sensitive sort - the argument is accepted without error but has no effect.

Related issues/PRs

This fix completes the scaffolding left unfinished in these merged PRs:

  • jinja : attribute support for join, map and sort #18883 - jinja : attribute support for join, map and sort (CISC, merged Jan 18, 2026): introduced the case_sensitive kwarg to the sort filter and left it commented out with FIXME. In review, CISC said "case_sensitive=False is default, but we are always case sensitive ATM" and promised a follow-up.
  • jinja : implement mixed type object keys #18955 - jinja : implement mixed type object keys (CISC, merged Jan 27, 2026): CISC's next PR, but addressed mixed type object keys, not case_sensitive. The promised follow-up never landed.
  • implement new jinja template engine #18462 - implement new jinja template engine (ngxson, merged Jan 16, 2026): the original PR that introduced the new Jinja engine, where the dictsort FIXME was also present.

No standalone issue was ever filed for this bug. The FIXME has been sitting in master for 5 months with no fix attempt.

Source locations

File: common/jinja/value.cpp

Site 1 - sort filter (value_array_t::get_builtins, line 1080):

Before (lines 1080-1081):

// FIXME: sorting is currently always case sensitive
//const bool case_sensitive = val_case->as_bool(); // undefined == false

After (line 1080 + new branch at line 1099):

const bool case_sensitive = val_case->as_bool(); // undefined == false

And inside the sort comparator, after attribute extraction, before the value_compare call:

if (!case_sensitive && is_val<value_string>(val_a) && is_val<value_string>(val_b)) {
    const std::string sa = val_a->as_string().lowercase().str();
    const std::string sb = val_b->as_string().lowercase().str();
    return reverse ? (sa > sb) : (sa < sb);
}

Site 2 - dictsort filter (value_object_t::get_builtins, line 1201):

Before (lines 1201-1202):

// FIXME: sorting is currently always case sensitive
//const bool case_sensitive = val_case->as_bool(); // undefined == false

After (line 1201 + unified comparator at line 1206):

const bool case_sensitive = val_case->as_bool(); // undefined == false

And the comparator refactored from if/else to unified val_a/val_b selection + same case-insensitive branch.

Local testing

Tested locally on Windows with pre-built llama-server (build b9747-d6d899580) and gemma-4-E2B-it-qat-UD-Q4_K_XL.gguf.

Before fix - probe template {{ ['Banana', 'apple', 'Cherry', 'date'] | sort(case_sensitive=false) | join(',') }} rendered via /apply-template endpoint:

Banana,Cherry,apple,date

Output is ASCII codepoint order - case_sensitive=false was ignored. Confirmed on the real binary.

After fix - logic simulation of the patched comparator (no compiler available locally to rebuild the DLL):

apple,Banana,Cherry,date

Matches Jinja2 spec for case-insensitive ascending sort.

Regression checks passed:

  • case_sensitive=true unchanged in both filters
  • reverse=true produces correct descending case-insensitive order
  • dictsort by=key and by=value paths both covered
  • llama-bench (pp512=39.62 t/s, tg128=10.72 t/s) and llama-perplexity baselines run without crash

Changes summary

  • Uncomment case_sensitive binding in both filters (2 lines removed, 2 lines added)
  • Add case-insensitive string compare branch in sort comparator (+5 lines)
  • Refactor dictsort comparator to unified val_a/val_b + same branch (net +4 lines)
  • Reuse existing .as_string().lowercase() (already used by lower filter, line 607) and .str() (already used in value_compare, line 1336) - no new helpers
  • case_sensitive=true and non-string comparisons fall through to existing value_compare unchanged

Tests added

File: tests/test-jinja.cpp

test_template(t, "sort case_sensitive=false",
    "{{ items|sort(case_sensitive=false)|join(',') }}",
    {{"items", json::array({"Banana", "apple", "Cherry", "date"})}},
    "apple,Banana,Cherry,date"
);

test_template(t, "sort case_sensitive=true",
    "{{ items|sort(case_sensitive=true)|join(',') }}",
    {{"items", json::array({"Banana", "apple", "Cherry", "date"})}},
    "Banana,Cherry,apple,date"
);

test_template(t, "dictsort case insensitive",
    "{% for k, v in obj|dictsort(case_sensitive=false) %}{{ k }}={{ v }} {% endfor %}",
    {{"obj", {{"Banana", 1}, {"apple", 2}, {"Cherry", 3}}}},
    "apple=2 Banana=1 Cherry=3 "
);

Test inputs use distinct lowercased values to avoid std::sort instability on ties.

Requirements

@dnislno
dnislno requested review from CISC and ggerganov as code owners June 24, 2026 12:46
@github-actions github-actions Bot added testing Everything test related jinja parser Issues related to the jinja parser labels Jun 24, 2026
@CISC

CISC commented Jun 24, 2026

Copy link
Copy Markdown
Member

This is not the right fix, don't bypass value_compare, instead make it support case_sensitive, and lowercase does not support unicode, so will not work correctly comparing non-ASCII characters.

@dnislno

dnislno commented Jun 24, 2026

Copy link
Copy Markdown
Author

This is not the right fix, don't bypass value_compare, instead make it support case_sensitive, and lowercase does not support unicode, so will not work correctly comparing non-ASCII characters.

Thanks for the review. You're right on both points. I'll revise to add a case_sensitive parameter to value_compare instead of bypassing it. For the unicode issue, I see unicode_tolower exists in src/unicode.* but common/ doesn't depend on src/ - do you want me to move it to common/, or keep ASCII-only with a TODO for now?

@CISC

CISC commented Jun 24, 2026

Copy link
Copy Markdown
Member

For the unicode issue, I see unicode_tolower exists in src/unicode.* but common/ doesn't depend on src/ - do you want me to move it to common/, or keep ASCII-only with a TODO for now?

Well, this is the main reason it has not been implemented yet (that and that there are no templates relying on this), just lowercasing is probably not enough and I'm not sure we want to move unicode handling. @ngxson any opinions on this?

@ngxson

ngxson commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

If this is not related to a reported issue, I don't think it's worth spending time. Sounds like a too nit-fix to me

@ngxson ngxson closed this Jun 25, 2026
@dnislno

dnislno commented Jun 25, 2026

Copy link
Copy Markdown
Author

Understood. Closing the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jinja parser Issues related to the jinja parser testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants