Exclude json-smart, not needed given how we are using json-path.#37
Exclude json-smart, not needed given how we are using json-path.#37xvrl merged 1 commit intometamx:masterfrom
Conversation
|
See also #34 |
There was a problem hiding this comment.
Can we exclude json-smart instead?
There was a problem hiding this comment.
I don't think so, since it's linked against by a static "DEFAULTS" in one of the jsonpath configuration classes.
There was a problem hiding this comment.
@gianm I think if we upgrade to JSONPath 2.1.0, we can exclude json-smart completely:
This PR is present in 2.1.0 and it skips the creation of the DefaultsImpl object if both a JsonProvider and MappingProvider are given to the builder before build() is called
There was a problem hiding this comment.
ok, that's an even better solution!
|
i m not sure if this is enough because no-one in their extensions can use asm version higher than 1.0.2 because net.minidev.asm needs to be included in the final deployments even after this PR for someone to be able to use json based input row parser |
|
@himanshug does it really? The json path tests still pass with this exclusion, I think it's only used by the json-smart impl for jsonpath but we aren't using that (it's the default impl but we don't use it). So you should be able to include whatever asm you want, or none at all. |
|
@gianm We tried excluding the net.minidev.asm dependency. But then got ClassNotFoundException in a realtime indexing task. |
|
@guobingkun it should work if you also have the change in this PR (avoid calling |
|
Another option is to avoid instantiating json-smart stuff altogether and then json-smart can be excluded. Unfortunately that involves tweaking a static field so it's probably better to do that in Druid than in java-util. The code would be: Configuration.setDefaults(
new Configuration.Defaults()
{
@Override
public JsonProvider jsonProvider()
{
return new JacksonJsonProvider();
}
@Override
public Set<Option> options()
{
return EnumSet.noneOf(Option.class);
}
@Override
public MappingProvider mappingProvider()
{
return new JacksonMappingProvider();
}
}
); |
|
we could probably add that to druid-api, doubt anyone would complain there. |
|
Configuration class you mentioned is from com.jayway.json-smart , and since the dependency for same is introduced in java-util here, i believe, the workaround can be added to java-util itself. |
|
@himanshug that Configuration class is from json-path, not json-smart. It seems weird to me to have a utils library making changes to static fields in other libraries, so it made more sense to me to put a workaround in druid or druid-api. we could also use a different json flattening library if you are aware of a better one. json-path was chosen somewhat arbitrarily (decent performance and features but not much stronger reason that than) |
|
@gianm my bad, yeah its in json-path but that is also introduced in java-util , where are you planning to put the change in druid-api or druid otherwise? |
There was a problem hiding this comment.
Bad for java8, please remove
There was a problem hiding this comment.
sorry, IntelliJ does that automatically when I hit the formatter. Any idea how to turn that off?
There was a problem hiding this comment.
preferences->editor->code style->java->javaDoc->generate <p/> on empty lines
that's the one I know about
There was a problem hiding this comment.
ok I'll give that a shot
gianm
force-pushed
the
exclude-minidev-asm
branch
3 times, most recently
from
7a0c20b to
4389ca0
Compare
|
@cheddar @himanshug @drcrallen @jon-wei updated with json-path 2.1.0 and excluded json-smart. |
gianm
changed the title
gianm
force-pushed
the
exclude-minidev-asm
branch
from
4389ca0 to
f72ae3a
Compare
|
👍 |
Exclude json-smart, not needed given how we are using json-path.
|
@gianm thanks for fixing this. @xvrl @drcrallen can one of you pls do a release? |
|
@himanshug done, 0.27.6 should be up on central in about 20min |
7 participants
See apache/druid#2193