Skip to content

Make sure all fields in sys.segments are JSON-serialized #10481

Merged
maytasm merged 6 commits intoapache:masterfrom
maytasm:IMPLY-4723
Oct 14, 2020
Merged

Make sure all fields in sys.segments are JSON-serialized #10481
maytasm merged 6 commits intoapache:masterfrom
maytasm:IMPLY-4723

Conversation

@maytasm
Copy link
Copy Markdown
Contributor

@maytasm maytasm commented Oct 5, 2020 •
edited
Loading

Make sure all fields in sys.segments are JSON-serialized

Description

This PR:

  • Make sure all fields in sys.segments are JSON-serialized. This is to be consistent with other sys tables such as spec in sys.supervisors. This is also more standardized and prevent things like changing Java class name from having end user impact (as our contract is to maintain compatibility of the JSON-serialized String).
  • Make all columns use the same casing

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 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.

suneet-s
suneet-s reviewed Oct 5, 2020
View reviewed changes
Comment thread sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java
.add("is_realtime", ValueType.LONG)
.add("is_overshadowed", ValueType.LONG)
.add("shardSpec", ValueType.STRING)
.add("shard_spec", ValueType.STRING)
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s Oct 5, 2020

Choose a reason for hiding this comment

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

shardSpec was introduced in this change #9883 and changing it to shard_spec would be a breaking change. I don't think we should do this without an upgrade plan

Copy link
Copy Markdown
Contributor Author

@maytasm maytasm Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think there is anything in Druid depending on shardSpec (similar to how the linked PR changed payload to shardSpec, metrics, dimensions). I can update the document but I don't think anything will break. I guess unless someone has a custom script reading this field which is probably unlikely and is why we should change this ASAP.

Copy link
Copy Markdown
Contributor

@suneet-s suneet-s Oct 5, 2020

Choose a reason for hiding this comment

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

The linked PR added those fields, so we didn't have to worry about backwards incompatibility.

@yuanlihan since you introduced this field, do you have any comments on changing the name of this field?

Copy link
Copy Markdown
Contributor Author

@maytasm maytasm Oct 5, 2020

Choose a reason for hiding this comment

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

The linked PR removed payload and added shardSpec, metrics, dimensions

@maytasm maytasm changed the title Imply 4723 Make sure all fields in sys.segments are JSON-serialized Oct 5, 2020
@suneet-s
Copy link
Copy Markdown
Contributor

suneet-s commented Oct 9, 2020 •
edited
Loading

Added Release Notes because this changes the behavior of an API - hopefully no one depends on it

suneet-s
suneet-s approved these changes Oct 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

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

LGTM

jon-wei
jon-wei approved these changes Oct 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jon-wei jon-wei left a comment

Choose a reason for hiding this comment

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

There are some real test failures and a spell check error, LGTM otherwise

Comment thread docs/querying/sql.md Outdated
|dimensions|STRING|The dimensions of the segment|
|metrics|STRING|The metrics of the segment|
|last_compaction_state|STRING|The configurations of the compaction task which created this segment. May be null if segment was not created by compaction task.|
|shard_spec|STRING|JSON-serialized of the segment `ShardSpec`|
Copy link
Copy Markdown
Contributor

@jon-wei jon-wei Oct 13, 2020

Choose a reason for hiding this comment

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

suggest "JSON-serialized form" instead here and elsewhere

Copy link
Copy Markdown
Contributor Author

@maytasm maytasm Oct 14, 2020

Choose a reason for hiding this comment

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

Done

@maytasm maytasm merged commit 3538abd into apache:master Oct 14, 2020
@jihoonson jihoonson added this to the 0.21.0 milestone Jan 4, 2021
@jihoonson jihoonson added the Bug label Jan 8, 2021
@jihoonson jihoonson mentioned this pull request Jan 13, 2021
Closed
JulianJaffePinterest pushed a commit to JulianJaffePinterest/druid that referenced this pull request Jan 22, 2021
@maytasm @JulianJaffePinterest
Make sure all fields in sys.segments are JSON-serialized (apache#10481)
7926e3b 
* fix JSON format

* Change all columns in sys segments to be JSON

* Change all columns in sys segments to be JSON

* add tests

* fix failing tests

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

Reviewers

@jon-wei jon-wei jon-wei approved these changes

@suneet-s suneet-s suneet-s approved these changes

Assignees

No one assigned

Labels

Bug Release Notes

Projects

None yet

Milestone

0.21.0

Development

Successfully merging this pull request may close these issues.

4 participants

@maytasm @suneet-s @jon-wei @jihoonson