SEPURI-SAI-KRISHNA commented on PR #42410:
URL: https://github.com/apache/superset/pull/42410#issuecomment-5113442697
Thanks for the review @rusackas — yes, tackled both. Pushed a follow-up.
**Copilot's `select.py` nit — name the missing columns**
Done, and it uses the interpolation style already present in the package
(`cum.py`
does `_("Invalid cumulative operator: %(operator)s", operator=operator)`):
```python
if missing := [column for column in exclude if column not in
df_select.columns]:
raise InvalidPostProcessingError(
_(
"Referenced columns not available in DataFrame: %(columns)s",
columns=", ".join(missing),
)
)
```
One wrinkle worth flagging, since it means the nit is only partly addressable
without widening scope. The generic wording I originally used wasn't
arbitrary —
it's the same string `validate_column_args` raises at `utils.py:136`. So the
decorator still intercepts the "column isn't in the frame at all" case
before my
guard runs, and that path keeps the unnamed message:
```
exclude=["abc"] -> Referenced columns not available
in DataFrame.
columns=["y"], exclude=["label"] -> Referenced columns not available
in DataFrame: label
```
I left the decorator's message alone deliberately: it's shared by every
operation
that uses `validate_column_args`, so renaming it is a broader change (and a
new
msgid for translators). The upside is the two messages now carry different
information — unnamed means "not in the query result", named means "your
`columns` selection removed it". Happy to make the decorator name its
columns too
if you'd rather they were uniform; it's a small change, just wider blast
radius
than I wanted to take unilaterally on an approved PR.
**Copilot's `query_object_test.py` nit — `.message` coupling**
Done, now `str(excinfo.value)`. Equivalent today —
`SupersetException.__init__` ends with `super().__init__(self.message)` —
but not
coupled to that staying true.
**One thing I noticed while verifying**
The two fixes overlap more than the PR description implies. Because my guard
checks the projected frame, and the projected frame is always a subset of the
input, the guard catches everything the decorator's `exclude` validation
would.
Reverting the decorator argname on its own now leaves the tests green;
reverting
the guard on its own fails with the original `KeyError`.
I'd still keep the decorator change — `"drop"` is not a parameter of
`select()`,
so leaving it is simply wrong and would silently mislead the next person —
but
the guard is the load-bearing half, and the description overstates the split.
Say the word if you'd like me to reword it.
Verified against the pinned toolchain: ruff 0.9.7, mypy 1.15.0 with the
hook's
stub set, pylint 3.3.7 with `superset.extensions.pylint` (10.00/10). Tests:
`pandas_postprocessing`, `queries` and `common` all pass (the two
`test_prophet`
failures are the optional `prophet` extra not being installed locally).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
