minor: refactor with assert_or_internal_err!() in datafusion/expr

[kumarUjjawal]

Which issue does this PR close?

• Part of [Tracking] Refactor with assert_or_internal_err!() across the codebase #18613

Rationale for this change

What changes are included in this PR?

Are these changes tested?

After switching those internal_err! guards to the new assertion macros, one test now fails. expr_rewriter::order_by::test::rewrite_sort_cols_by_agg_alias expects the
rewritten sort expression to stay as a fully qualified column parsed from col("min(t.c2)"), but the new code returns an unqualified Column { relation: None, name: "min(t.c2)" }, so the test’s equality check breaks. I would appreciate any feedback for moving forward with this.

Are there any user-facing changes?

[Jefffrey]

I expect these refactors to change no behaviour in regards to the tests (would prefer not to change the tests)

[kumarUjjawal]

I expect these refactors to change no behaviour in regards to the tests (would prefer not to change the tests)

Any suggestion for what I am doing wrong?

[Jefffrey]

I expect these refactors to change no behaviour in regards to the tests (would prefer not to change the tests)

Any suggestion for what I am doing wrong?

I would say revert the changes that are causing the tests to fail

[2010YOUY01]

From the CI log I only see sqllogcitest failures, and it's just error message change due to the new macro

You can apply auto fix with https://datafusion.apache.org/contributor-guide/testing.html#sqllogictests-tests

[kumarUjjawal]

Closing this in favor of #18731

[Jefffrey]

Closing this in favor of #18731

Please keep in mind we prefer keeping existing PRs instead of closing and opening new ones; it causes us to lose context and makes it harder for us to keep track (especially as for this tracking issue we have a lot of PRs to keep track of in a short span of time)

[kumarUjjawal]

Closing this in favor of #18731

Please keep in mind we prefer keeping existing PRs instead of closing and opening new ones; it causes us to lose context and makes it harder for us to keep track (especially as for this tracking issue we have a lot of PRs to keep track of in a short span of time)

I understand but i made a mess in this one when I ran the cargo test --test sqllogictests -- --complete command for the test and it changed several files. The CI was still failing and it was becoming harder for me to track what broke it. If you could help track it down, it would be great help. Sorry for the inconvenience.

[Jefffrey]

Closing this in favor of #18731

Please keep in mind we prefer keeping existing PRs instead of closing and opening new ones; it causes us to lose context and makes it harder for us to keep track (especially as for this tracking issue we have a lot of PRs to keep track of in a short span of time)

I understand but i made a mess in this one when I ran the cargo test --test sqllogictests -- --complete command for the test and it changed several files. The CI was still failing and it was becoming harder for me to track what broke it. If you could help track it down, it would be great help. Sorry for the inconvenience.

As I mentioned before, I don't expect test changes to be necessary with these refactors. Please be conservative in the refactors being done and run tests as you do the refactor to ensure we don't break them.

[kumarUjjawal]

run tests as you do the refactor to ensure we don't break them.

Thanks! I will try this.