Skip to content

Improve the wording around the InvalidNullByteException#13794

Closed
LakshSingla wants to merge 2 commits intoapache:masterfrom
LakshSingla:nullbyte-error-msg
Closed

Improve the wording around the InvalidNullByteException#13794
LakshSingla wants to merge 2 commits intoapache:masterfrom
LakshSingla:nullbyte-error-msg

Conversation

@LakshSingla
Copy link
Copy Markdown
Contributor

Description

This PR improves the wording around the mysterious InvalidNullByteException. The exception occurs when the strings that are added to the frame contain the 0x0000 byte which is internally being used as a delimiter in the case of a string column. The current use case for the frames is MSQ exclusively, and this error will only be generated if the ingested external data contains hidden null bytes, which in most cases can safely be sanitized.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Copy link
Copy Markdown
Contributor

@abhishekagarwal87 abhishekagarwal87 left a comment

Choose a reason for hiding this comment

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

thank you for making this error message more actionable.

if (!allowNullBytes && b == 0) {
throw new InvalidNullByteException();
throw new InvalidNullByteException(
"Unable to add the frame because it contains null bytes. This usually happens when the added string columns "
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.

Thanks for the improved message. The user, however, knows nothing about frames. Can we word this from the user's perspective?

Druid does not support null (0x00) bytes in strings. File %s, row $d, column %s contains null bytes: [%s].

The string would be encoded so that control characters appear as \U0000 so the user can see the position of the null bytes.

Maybe we don't know the row number (in a form useful to the user.) If not, just list the column.

This is a case of unparsable data. Should we have caught it at the time we read the data rather than when writing to a frame? Should we invoke our bad-row logic to skip this row? That logic should log the bad row for later re-ingestion, but I don't think we've added that ability.

If we catch the problem on read, then the check here is more of an assertion. Though, perhaps the data was created by an expression, so it is still worth validating.

@github-actions
Copy link
Copy Markdown

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

@github-actions github-actions Bot added the stale label May 12, 2023
@LakshSingla
Copy link
Copy Markdown
Contributor Author

Raised an improved PR tackling this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants