[FEATURE][ML] Parse results and join them in the data-frame copy index #36382
Conversation
|
Pinging @elastic/ml-core |
droberts195
left a comment
droberts195
left a comment
There was a problem hiding this comment.
Looks good - I just noted a few minor things
| * end of data message. | ||
| */ | ||
| private static final String END_OF_DATA_MESSAGE_CODE = "r"; | ||
| private static final String END_OF_DATA_MESSAGE_CODE = "$"; |
There was a problem hiding this comment.
I think it's still 'r' on the C++ side.
There was a problem hiding this comment.
This https://github.com/elastic/ml-cpp/pull/334/files changes it to $
| process.flushStream(); | ||
|
|
||
| LOGGER.debug("[{}] Closing process", jobId); | ||
| LOGGER.info("[{}] Waiting for result processor to complete", jobId); |
There was a problem hiding this comment.
I don't think we'll want this to be an INFO in production. If you want to leave it like this on the feature branch please add a TODO to downgrade it.
There was a problem hiding this comment.
Indeed, this shouldn't be an info message. I left it on purpose as I think it's useful during development. I had in mind that we'd review all logging when this is refactored into persistent tasks. Not sure I'd have a todo for each one of them, but I can add them if you think it's best.
| } catch (IOException e) { | ||
| LOGGER.error(new ParameterizedMessage("[{}] Error writing data to the process", jobId), e); | ||
| } finally { | ||
| LOGGER.info("[{}] Closing process", jobId); |
There was a problem hiding this comment.
Ditto either downgrade this now or add a TODO to do so before merging the feature branch.
| } | ||
| AnalyticsResult result = currentResults.get(i); | ||
| SearchHit hit = row.getHit(); | ||
| Map<String, Object> source = new HashMap(hit.getSourceAsMap()); |
There was a problem hiding this comment.
Do we want the new fields to come at the end of the source document? If so, this should use LinkedHashMap instead of HashMap. (Maybe we don't care though.)
There was a problem hiding this comment.
Good question. I am not sure what is actually affected by the order. But I think it makes sense to append them at the end as the least invasive annotation of the original rows.
| public Iterator<AutodetectResult> parseResults(InputStream in) throws ElasticsearchParseException { | ||
| public class ProcessResultsParser<T> { | ||
|
|
||
| private static final Logger LOGGER = LogManager.getLogger(ProcessResultsParser.class); |
There was a problem hiding this comment.
I think the tradition is lowercase logger for static loggers, as the object is not immutable.
| inOrder.verify(lengthEncodedWriter).writeNumFields(4); | ||
| inOrder.verify(lengthEncodedWriter, times(3)).writeField(""); | ||
| inOrder.verify(lengthEncodedWriter).writeField("r"); | ||
| inOrder.verify(lengthEncodedWriter).writeField("$"); |
dimitris-athanasiou
deleted the
parse-results-and-join-them-in-data-frame-copy-index
branch
3 participants
No description provided.