Skip to content

Parser interface docs; deprecate setFieldNames/getFieldNames.#35

Merged
drcrallen merged 1 commit intometamx:masterfrom
gianm:parser-docs
Nov 9, 2015
Merged

Parser interface docs; deprecate setFieldNames/getFieldNames.#35
drcrallen merged 1 commit intometamx:masterfrom
gianm:parser-docs

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Nov 6, 2015

  • Docs for Parser
  • Deprecated setFieldNames/getFieldNames. They don't appear to be used in meaningful ways anywhere in Druid, and I think are just confusing things by being on the interface.

@gianm gianm mentioned this pull request Nov 6, 2015
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.

actually, thinking more about it, it seems support for fieldNames is useful for csv, tsv and the regex, javascript parsers in #36 unlike the JSONParser and they shouldn't be deprecated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@himanshug the thought behind deprecating these was:

  • external usages of setFieldNames are generally immediately after construction of a specific parser class. So those usages don't really need to use a setter- they could instead use a constructor that offers fieldNames (those constructors already exist but are not always used).
  • these methods are actually pretty nonsensical for the JsonPath parser (Add JsonPath parser #34), because its FieldSpecs serve the same purpose, but they do it in a richer way that is specific to JsonPath.

So, I don't think these methods are really useful, nor do they generalize well to all parsers.

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.

ok, sounds reasonable, I agree field names do not make sense for json parsers but only for csv, tsv, regex parsers who can support those via specific constructors.

@himanshug
Copy link
Copy Markdown
Contributor

👍

@xvrl @nishantmonu51 @drcrallen can one of you pls merge this?

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Nov 9, 2015

👍

drcrallen added a commit that referenced this pull request Nov 9, 2015
Parser interface docs; deprecate setFieldNames/getFieldNames.
@drcrallen drcrallen merged commit 6d0d9c1 into metamx:master Nov 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants