-
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
add explodeSpec #8698
add explodeSpec #8698
Conversation
@clintropolis @vogievetsky Since the |
Would you please check the build failure? It looks legit.
|
@@ -77,8 +82,22 @@ public InputRow nextRow() throws IOException | |||
if (!hasMore()) { | |||
throw new NoSuchElementException(); | |||
} | |||
if (!parsedInputRows.isEmpty()) { | |||
return parsedInputRows.remove(0); |
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.
ArrayList.remove()
internally moves the positions of remaining elements and this will happen for every row in this case which could be expensive. Since parsedInputRows
won't be large in general, I suggest to use an iterator on the list instead of modifying the list.
public E remove(int index) {
rangeCheck(index);
modCount++;
E oldValue = elementData(index);
int numMoved = size - index - 1;
if (numMoved > 0)
System.arraycopy(elementData, index+1, elementData, index,
numMoved);
elementData[--size] = null; // clear to let GC do its work
return oldValue;
}
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.
Cool, it's a good idea, I was worrying about this issue too.
Tried to use LinkedList so that's O(1) pop, but I wonder why it's a forbidden API?
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's because usually LinkedList
is not a good idea. See #6112.
}; | ||
} | ||
|
||
public interface ExploderMaker<T> |
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.
Would you please add javadoc to this class and methods? For class, It should include what this is and where it is used. For methods, it should say what the method does, what inputs and outputs are.
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.
sure
{ | ||
List<Object> getExplodeArray(T node, String expr); | ||
|
||
T setObj(T node, Object value, String expr); |
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.
I.. don't know what the best name is for this method, but setObj()
sounds not intuitive enough. Maybe createNewNodeWithValue()
? Or you can split this into two methods of copy()
and setValue()
.
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.
I will think of more intuitive names for those two methods.
@@ -82,6 +82,7 @@ | |||
null | |||
), | |||
new JSONPathSpec(true, ImmutableList.of()), | |||
null, |
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.
Should there be a sampler end-to-end unit test with an explodeSpec
?
@SEKIRO-J this has a bunch of conflicts |
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions. |
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Description
Add explodeSpec feature, to give user ability to explode array objects without writing their own pre-processing scripts.
trying to add a forbidden-apis
in
FileIteratingFirehose
, the fieldprivate final ArrayList<InputRow> parsedInputRows;
should be better declared asLinkedList
, as consequential operation involves lots ofpush
andpop
, so better to use a queue.ArrayDeque
doesn't work here because we need to insertnull
This PR has:
Key changed/added classes in this PR
JSONExplodeSpec
JSONExploderMaker
ObjectExploder
ObjectExploders