Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show errant values in current-domain out-of-bounds error message #5421

Merged
merged 5 commits into from
Jan 10, 2025

Conversation

johnkerl
Copy link
Contributor

@johnkerl johnkerl commented Jan 9, 2025

[sc-61579]

Before:

tiledbsoma._exception.SOMAError: [ManagedQuery] [unnamed] Query FAILED: Query: A range was set outside the current domain.

After:

tiledbsoma._exception.SOMAError: [ManagedQuery] [unnamed] Query FAILED: Query: A range [99999, 99999] on dimension 'soma_joinid' was set outside of the current domain [[0, 2637]].

TYPE: IMPROVEMENT
DESC: Show errant values in current-domain out-of-bounds error message

@johnkerl johnkerl requested a review from ihnorton January 9, 2025 20:12
"domain {}.",
range_str(
range, array_schema_->domain().dimension_ptr(d)->type()),
array_schema_->domain().dimension_ptr(d)->name(),
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be current_domain()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the domain to get types. Those are wired through array_schema_ and current_domain both. I can get them either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ihnorton correction: the domain is wired through the array_schema_ and the private ndrectangle_ of CurrentDomain.

It is an invariant that they be the same domain in both places.

If you want to pursue the latter then we need to connect a domain accessor in CurrentDomain which passes through the copy of the domain in its ndrectangle_. Personally I don't see the benefit to doing the latter.

@johnkerl
Copy link
Contributor Author

johnkerl commented Jan 9, 2025

Even though we moved to C++ 20 a long time ago, it appears that in CI there are lots of builds using C++ 17. And I've introduced std::format on this PR. Hence all the red CI.

@ihnorton @teo-tsirpanis your thoughts please:

  • I can use fmt::format
  • I can std::stringstream ss, ss << "this", ss << "that", etc. & then throw QueryException(ss.str())

@teo-tsirpanis
Copy link
Member

it appears that in CI there are lots of builds using C++ 17

That's definitely not the case, we use C++20 language features like concepts without any problem. It looks like the compiler versions we are using do not have the library implemented.

Better use fmt instead.

@johnkerl johnkerl force-pushed the kerl/current-domain-error-message branch from 34d9883 to bc2d629 Compare January 9, 2025 21:00
@johnkerl johnkerl force-pushed the kerl/current-domain-error-message branch from 2f70b54 to c97a166 Compare January 9, 2025 22:43
Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

Thanks!

teo-tsirpanis added a commit that referenced this pull request Jan 10, 2025
Found while debugging CI fails on #5421.

---
TYPE: NO_HISTORY

---------

Co-authored-by: Theodore Tsirpanis <[email protected]>
@teo-tsirpanis teo-tsirpanis merged commit 16c96b2 into main Jan 10, 2025
59 checks passed
@teo-tsirpanis teo-tsirpanis deleted the kerl/current-domain-error-message branch January 10, 2025 15:20
@johnkerl
Copy link
Contributor Author

Thank you @teo-tsirpanis !!

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.

3 participants