[ML-Dataframe] Feature/dataframe basictests #32783
Conversation
|
Pinging @elastic/ml-core |
benwtrent
left a comment
benwtrent
left a comment
There was a problem hiding this comment.
Some minor initial comments.
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(state); |
| builder.field("id", id); | ||
| { | ||
| builder.field(ID, id); | ||
| } |
There was a problem hiding this comment.
Why are you keeping the scope if there is no longer a branch that prevents entry?
There was a problem hiding this comment.
I agree, took this over from another place and was wondering myself, but then thought:
As this builds a json object, it emulates the json structure. So this is rather syntactic sugar: forcing indentation to make the code more readable.
(Right now the whole config is bogus (empty). But this will be the next step.)
| import org.elasticsearch.common.xcontent.XContentBuilder; | ||
| import org.elasticsearch.common.xcontent.XContentParser; | ||
| import org.elasticsearch.xpack.core.XPackPlugin; | ||
| import org.elasticsearch.xpack.core.rollup.job.RollupJobConfig; |
| PARSER = new ConstructingObjectParser<>(NAME, false, (args, optionalId) -> { | ||
| String id = args[0] != null ? (String) args[0] : optionalId; | ||
| return new FeatureIndexBuilderJobConfig(id); | ||
| }); |
There was a problem hiding this comment.
This does not necessarily need to be within a static initialization block.
There was a problem hiding this comment.
do not get this, can you elaborate?
There was a problem hiding this comment.
Usually, the static variable initialization is done outside of the static block.
Though this is not a strict pattern, I just don't see why you chose to initialize the value here instead of at the declaration.
|
|
||
| private String id; | ||
| private static final String NAME = "xpack/feature_index_builder/jobconfig"; | ||
| private static final String ID = "id"; |
There was a problem hiding this comment.
Why did not remove this from being a ParseField? This seems to go against the prevailing pattern.
There was a problem hiding this comment.
no strong feeling, I saw both versions in code
hendrikmuhs
changed the title
hendrikmuhs
changed the title
|
retest this please |
|
The remaining CI issues are due to the incomplete integration. Some classes need to be moved into X-Pack core, best to do this together with the planned naming changes and best to be done in a separate PR. |
3 participants
Feature Index PR
this change enables testing as well as adds basic tests