Skip to content

feat: add operator parameter to vf() for range filters#1813

Open
jacalata wants to merge 1 commit into
tableau:developmentfrom
jacalata:jac/vf-range-filter-operators
Open

feat: add operator parameter to vf() for range filters#1813
jacalata wants to merge 1 commit into
tableau:developmentfrom
jacalata:jac/vf-range-filter-operators

Conversation

@jacalata

Copy link
Copy Markdown
Contributor

Summary

  • Adds optional operator parameter to RequestOptions.vf() (and by inheritance PDFRequestOptions, ImageRequestOptions, etc.)
  • When provided, encodes the filter as operator:value (e.g. gt:1000) matching the REST API's range filter syntax
  • Backwards-compatible: omitting operator preserves existing exact-match behaviour

Closes #1752

Test plan

  • test_vf_with_operator in test/test_request_option.py verifies gt:1000 encoding
  • Existing vf tests continue to pass (exact-match unchanged)

🤖 Generated with Claude Code

Exposes comparison operators (GreaterThan, LessThan, etc.) on the
vf() method of _DataExportOptions. The REST API accepts operator:value
encoding for view filters; previously callers had to hand-craft the
string themselves.

Fixes tableau#1752

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bcantoni

Copy link
Copy Markdown
Contributor

Code Review

Summary: Adds an optional operator parameter to _DataExportOptions.vf() (inherited by PDFRequestOptions, ImageRequestOptions, etc.), encoding filters as operator:value (e.g. gt:1000) instead of a bare value. Backwards compatible when omitted. Closes #1752.

Mechanical checks (on the PR branch): tests pass (42/42), black --check clean, mypy clean.

🔴 Critical: the feature doesn't do what it claims

Tableau's REST API documentation for view filters (vf_*) explicitly states:

"Wild cards, value ranges, and non-equivalencies, like contains, less than, or greater than, aren't supported."

View filters (vf_field=value) only ever do exact string match server-side — unlike the separate filter=field:operator:value endpoint-filtering syntax (used elsewhere in this codebase via Filter/RequestOptions.filter), which does support operators. This PR conflates the two mechanisms. Sending vf_sales=gt:1000 will not filter for Sales > 1000 — the server will look for rows where the field literally equals the string "gt:1000", silently returning an empty/wrong result set rather than erroring. The linked issue (#1752) proposes this same approach, so the misunderstanding originates there.

Given this, I don't think this should merge as-is — it adds a plausible-looking but non-functional API surface that would mislead callers into believing range filtering works when it doesn't. It'd be worth checking with the API/docs team (or re-triaging #1752) on whether there's a different mechanism (e.g. a different endpoint, or dashboard-level filter configuration) that actually supports range filters for PDF/image exports, since .vf() on view filters isn't it.

Other findings

  • Test coverage gap: the new test_vf_with_operator test only asserts the client-side string built (gt:1000 appears in the outgoing query param) via requests_mock — it can't and doesn't catch that the real server ignores/misinterprets this syntax.
  • No input validation: unlike query.py's _parse_shorthand_filter (which raises ValueError for an unrecognized operator), vf() accepts any string as operator with no validation against RequestOptions.Operator.

🤖 Generated with Claude Code

@bcantoni

Copy link
Copy Markdown
Contributor

@jacalata not sure how to interpret claude's review findings. Is it missing the fix or do the docs need to updated?

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.

[Type 2] Support range filter operators (GreaterThan, LessThan, Between) in PDFRequestOptions.vf() for view filters

2 participants