-
Notifications
You must be signed in to change notification settings - Fork 90
feat: DH-10139: Introduce structured errors for input table updates. #7113
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
feat: DH-10139: Introduce structured errors for input table updates. #7113
Conversation
c85adfa to
7c93440
Compare
mofojed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we could get a structured object back with the errors that would be better long-term; not only could we highlight the cells that have the errors, it would be essential if we ever want to internationalize our application.
Would eliminate much of the string building/formatting going on in this PR.
7c93440 to
a5dba61
Compare
No docs changes detected for 67fbe8f |
| * </p> | ||
| * . | ||
| */ | ||
| public interface StructuredError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this needs to be an interface? Seems like we can just have a concrete class (like StructuredErrorImpl).
| GrpcUtil.safelyError(responseObserver, StatusProto.toStatusException(status)); | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this compare to throw StatusProto.toStatusRuntimeException(status);?
server/src/main/java/io/deephaven/server/table/inputtables/RangeValidatingInputTable.java
Show resolved
Hide resolved
| * </p> | ||
| */ | ||
| @TestUseOnly | ||
| public class RangeValidatingInputTable implements InputTableUpdater { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call make with an input table and an integer column and a min and a max
that's the easiest way to make one, start testing it out
engine/table/src/main/java/io/deephaven/engine/util/input/InputTableValidationException.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/util/input/InputTableValidationException.java
Outdated
Show resolved
Hide resolved
proto/proto-backplane-grpc/src/main/proto/deephaven_core/proto/inputtable.proto
Show resolved
Hide resolved
…tTableValidationException.java Co-authored-by: Colin Alworth <[email protected]>
…tTableValidationException.java Co-authored-by: Colin Alworth <[email protected]>
…ight/deephaven-core into nightly/cpw/input-table-experiments
| # | ||
| # Copyright (c) 2016-2025 Deephaven Data Labs and Patent Pending | ||
| # | ||
|
|
||
| """Demo how to chain table operations.""" | ||
|
|
||
| from pydeephaven import Session, Table, DHError | ||
| from pydeephaven.table import InputTable | ||
| import pyarrow as pa | ||
| from datetime import datetime | ||
| import time | ||
| from grpc_status import rpc_status | ||
| import grpc | ||
| from google.protobuf.any_pb2 import Any | ||
| from deephaven_core.proto.inputtable_pb2 import InputTableValidationErrorList | ||
|
|
||
| err = None | ||
| stat = None | ||
| err_list = None | ||
|
|
||
| def main(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- doc string is wrong
- do we want to add a test case in addition to this example?
- is formal documentation needed ( a link to the demo could suffice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I updated the docstring to be more helpful.
- has a test of an input table addition already
keyed_input_t = self.session.input_table(schema=schema, key_cols="f1") - There are no production grade validations available from Core. The input table operations remain compatible, without any need to change the Python documentation.
| except DHError as dhe: | ||
| err = dhe | ||
| parent = dhe.__cause__ | ||
| while parent is not None: | ||
| if isinstance(parent, grpc.Call): | ||
| stat = rpc_status.from_call(parent) | ||
| parent = None | ||
|
|
||
| for detail in stat.details: | ||
| if detail.type_url == "type.googleapis.com/" + InputTableValidationErrorList.DESCRIPTOR.full_name: | ||
| el = InputTableValidationErrorList() | ||
| if detail.Unpack(el): | ||
| err_list = el | ||
| else: | ||
| print("Could not unpack: " + detail) | ||
| else: | ||
| parent = parent.__cause__ | ||
|
|
||
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a public helper function in the API to wrap all these parsing justified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is just to prove that we could extract them if we wanted. There is a formatted message passed along in the error string. But if we are to use the googlerpc status library in more places, we do want to make sure that the languages we care about work.
jmao-denver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python changes LGTM
No description provided.