Skip to content

fix: handle param overlap between model and its dotted child - #1901

Open
ilongin wants to merge 8 commits into
mainfrom
ilongin/1883-fix-param-overlap-index-error
Open

fix: handle param overlap between model and its dotted child#1901
ilongin wants to merge 8 commits into
mainfrom
ilongin/1883-fix-param-overlap-index-error

Conversation

@ilongin

@ilongin ilongin commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #1883.

When .map(params=[...]) (or to_iter(...)) asked for both a model and one of its dotted children, e.g. params=["fr.name", "fr"], we hit IndexError when building the UDF input row.

Root cause: to_udf_spec dedupes shared columns (dict keyed by db-flat name), so the row has one copy of the shared leaf. row_to_objs walked a linear pos counter and expected two copies. row_to_features has the same shape - not user-visible today because db_signals doesn't dedupe, but same fragile pattern.

Both now build a {db_name -> row_index} map and look up each value's columns by name.

  • row_to_objs: positions built from to_udf_spec().
  • row_to_features: positions built from db_signals() (dupes still work - dict overwrite is a no-op since both indices hold the same column value).
  • New helper _sub_row_for_model assembles the per-model sub-row, handling the _type_tag sentinel for nested Optional[Model].

Tests: unit tests for row_to_objs and row_to_features covering scalar/nested/optional overlap, plus e2e coverage via .map() and to_iter.

When map() params (or to_iter columns) include both a model and one of
its dotted children (e.g. params=["fr.name", "fr"]), row_to_objs walked
a linear pos counter over row values whose columns had already been
deduped by to_udf_spec, causing IndexError. Same latent shape in
row_to_features. Both now look up positions by db-flat name.
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes parent/child parameter overlap during row hydration.

Changes:

  • Replaces positional unpacking with DB-column name lookups.
  • Adds shared model sub-row assembly with optional-model sentinel support.
  • Adds unit and end-to-end overlap tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/datachain/lib/signal_schema.py Implements name-based row hydration.
tests/unit/lib/test_signal_schema.py Tests overlapping schema values.
tests/unit/lib/test_datachain.py Tests .map() and to_iter() behavior.
Suppressed comments (1)

src/datachain/lib/signal_schema.py:765

  • This recomputes db_signals() and rebuilds the positions dictionary for every yielded row, although to_iter() already computes the same DB signal list once before entering its row loop. On large iterations this adds a full schema traversal and dictionary allocation per record; compute the mapping once per schema/iterator and reuse it.
        positions: dict[str, int] = {
            str(name): i for i, name in enumerate(self.db_signals())
        }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/datachain/lib/signal_schema.py Outdated
Comment thread src/datachain/lib/signal_schema.py Outdated
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 24, 2026

Copy link
Copy Markdown

Deploying datachain with  Cloudflare Pages  Cloudflare Pages

Latest commit: a63f6de
Status: ✅  Deploy successful!
Preview URL: https://19390d81.datachain-2g6.pages.dev
Branch Preview URL: https://ilongin-1883-fix-param-overl.datachain-2g6.pages.dev

View logs

return res

@cached_property
def _udf_row_positions(self) -> dict[str, int]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI generated as an attempt to simplify this:

_parse_row() already has a RowDict keyed by these flattened DB names, so converting it to a list and then reconstructing name→position caches seems avoidable. Could we pass that mapping directly to row_to_objs() and gather model subrows by their ordered DB column names? For sequence compatibility, row_to_objs() could normalize sequences with dict(zip(self.to_udf_spec(), row, strict=True)).
This would remove the four position caches and _model_row_positions(), while leaving row_to_features() and _hydrate_model() unchanged. Feature rows retain duplicate columns and already handle parent/child overlap correctly; the new to_iter/row_to_features overlap tests also pass on main.

@shcheklein shcheklein left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please check if we can simplify the implementation

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.

IndexError when a map reads both a parent object and a nested field of it

3 participants