-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Data loader (GUI component) #7572
Conversation
Might change some copy soon but felt cute in this gif: https://imgur.com/a/nvzCs4V (too big for GH embed) |
db6702b
to
b2bcd9c
Compare
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.
lgtm 👍
type: 'index', | ||
spec: { | ||
ioConfig: deepSet(ioConfig, 'type', 'index') | ||
// dataSchema: { |
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.
why is this block commented?
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.
That will be commented in to support #7566
tuningConfig?: TuningConfig; | ||
} | ||
|
||
export type IngestionType = 'kafka' | 'kinesis' | 'index_hadoop' | 'index' | 'index_parallel'; |
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.
It seems like it would be really useful in the future to figure out a way to populate this stuff based on the set of loaded extensions, but I'm not sure the best mechanism to do that at this point.
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.
Well that is just a type, so in the future it would probably become string
. I was planning of proposing a system for extending the data loader to arbitrary extensions. So that every extension could declare a json blob (that I will propose) and then be able to 'register' itself with the data loader
} | ||
]; | ||
|
||
it('works for path, ignore-arrays', () => { |
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.
heh, nice 👍
{!selectedMetricSpec && this.renderDimensionSpecControls()} | ||
{!selectedDimensionSpec && this.renderMetricSpecControls()} | ||
{this.renderChangeRollupAction()} | ||
{this.renderChangeDime |
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.
nit: this reads strangely, but it's just taken from the docs...
{!selectedMetricSpec && this.renderDimensionSpecControls()} | ||
{!selectedDimensionSpec && this.renderMetricSpecControls()} | ||
{this.renderChangeRollupAction()} | ||
{this.renderChangeDime |
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.
nit: same about reading a bit strange, but also copied from the docs...
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.
Yeah I was going to submit an issue about that part being a bit unclean in the druid ingestion spec.
|
||
// copied from http://goo.gl/0ejHHW with small tweak to make dddd not pass on its own | ||
// tslint:disable-next-line:max-line-length | ||
export const ISO_MATCHER = new RegExp(/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))(T((([01]\d|2[0-3])((:?)[0-5]\d)?|24:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)$/); |
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.
oh man, dates and times are just the worst 🙃
"version": "7.3.4", | ||
"resolved": "https://registry.npmjs.org/@babel/core/-/core-7.3.4.tgz", | ||
"integrity": "sha512-jRsuseXBo9pN197KnDwhhaaBzyZr2oIcLHHTt2oDdQrej5Qp57dCCJafWx5ivU8/alEYDpssYqv1MUqcxwQlrA==", | ||
"version": "7.4.3", |
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.
just leaving a comment to call out that we'll need to update versions in the license stuff for next release
This PR implements the GUI part of #7502 and depends on #7531 being merged.
The data loader / spec editor is fully functional for
index
andindex_parallel
tasks and with a few more changes and also #7566 being merged it will be able to handle streaming data also (but there is no need to wait for #7566).Since this PR required so much UI work a lot of other goodies in the console were updated to accommodate it:
AutoForm
to be flexible enough for all the new use-cases in the codeSQL
toQuery
so it is a verb)npm start
script APIHere is a screenshot of the Schema design page: