Skip to content

Comments

escape smoosh file names, fix bug with empty json paths#13079

Closed
clintropolis wants to merge 1 commit intoapache:masterfrom
clintropolis:smoosh-fix
Closed

escape smoosh file names, fix bug with empty json paths#13079
clintropolis wants to merge 1 commit intoapache:masterfrom
clintropolis:smoosh-fix

Conversation

@clintropolis
Copy link
Member

Description

This PR fixes some issues encountered with nested column objects with key names containing newlines or commas, which while valid JSON, are not cool with the way the meta.smoosh file currently works.

To remedy this, the smoosh file now escapes commas and newlines when writing the meta.smoosh file, and unescapes them upon mapping.

This PR also fixes an issue with the jsonpath and jq parsers when handling empty keys, which are also valid JSON property names. They were both a bit overly strict, and now should allow them if contained in syntax appropriate quotes.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • 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.
  • been tested in a test Druid cluster.

*/
public class SmooshedFileMapper implements Closeable
{
private static final String COMMA = "\u002c";
Copy link
Contributor

Choose a reason for hiding this comment

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

is it used anywhere?

Comment on lines +200 to +203
'0' == fileName.charAt(i + 1) &&
'0' == fileName.charAt(i + 2) &&
'2' == fileName.charAt(i + 3) &&
'c' == fileName.charAt(i + 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we are are exceeding the string length here?

@github-actions
Copy link

github-actions bot commented Jan 9, 2024

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
Copy link

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
Copy link

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, 2024
@github-actions
Copy link

github-actions bot commented Jun 9, 2024

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants