Skip to content

[IOTDB-4335] Implement PathPatternTree to support trigger and delete#7270

Merged
qiaojialin merged 16 commits intoapache:masterfrom
Cpaulyz:IOTDB-4335
Sep 17, 2022
Merged

[IOTDB-4335] Implement PathPatternTree to support trigger and delete#7270
qiaojialin merged 16 commits intoapache:masterfrom
Cpaulyz:IOTDB-4335

Conversation

@Cpaulyz
Copy link
Copy Markdown
Contributor

@Cpaulyz Cpaulyz commented Sep 8, 2022

Description

private final String name;
private final Map<String, PathPatternNode> children;
private final Map<String, PathPatternNode<V>> children;
private Set<V> valueSet;
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.

It seems that you forgot about serializing this field.

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.

And equalsWIth may also take it into account

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.

It seems that you forgot about serializing this field.

Nope. I reuse the data structure of the nodes in PathPatternTree here, mounting an additional valueSet. Serialization methods are only used in PathPatternTree, and the serialization information is used as a ByteBuffer in RPC. valueSet is always empty in PathPatternTree, so I did not serialize it to save network cost .

And equalsWIth may also take it into account

I've added the valueSet comparison in equalsWith

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.

I think it's better to do the serialization in PathPatternNode, because PatternTreeMap also need to be serialized and tranferred by rpc.
It's fine to occupy four more bytes(int size) for one node to indicate the valueSet's size, one packet in network will be like 1KB or something else, four more bytes per node will not hurt the perfermance.

Comment on lines +143 to +144

private void searchDevicePattern(
PathPatternNode curNode, List<String> nodes, Set<String> results) {
PathPatternNode<Void> curNode, List<String> nodes, Set<String> results) {
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.

What if root.sg.d1.**, it seems that this method won't put it into result.

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.

I didn't change the semantics of this interface. It originally seemed to assume that the device of root.sg1.d1.** is root.sg1.d1.**. I didn't know if the modification will have an impact on the upper layer application

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.

You can discuss with @liuminghui233 and @MarcosZyk about it, I think it's a bug in previous implementation, and can be fixed by the way in this pr.

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.

The device of root.sg1.d1.** may be root.sg1.d1 and root.sg1.d1.**.

String[] pathNodes,
int pos,
List<V> resultList,
boolean fromMultiWildCard) {
Copy link
Copy Markdown
Contributor

@JackieTien97 JackieTien97 Sep 15, 2022

Choose a reason for hiding this comment

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

fromMultiWildCard means there may exist two consecutive ** in PatternTreeMap?
I think this case should be deal with or avoided at the very beginning like during constructing this PatternTreeMap.

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.

fromMultiWildCard is unnecessary. I will remove it later.

Comment on lines +35 to +36
private BiConsumer<V, Set<V>> appendFunction;
private BiConsumer<V, Set<V>> deleteFunction;
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.

Suggested change
private BiConsumer<V, Set<V>> appendFunction;
private BiConsumer<V, Set<V>> deleteFunction;
private final BiConsumer<V, Set<V>> appendFunction;
private final BiConsumer<V, Set<V>> deleteFunction;

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.

done

Copy link
Copy Markdown
Contributor

@JackieTien97 JackieTien97 left a comment

Choose a reason for hiding this comment

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

Excellent job!

@qiaojialin qiaojialin merged commit 75ee2f3 into apache:master Sep 17, 2022
@Cpaulyz Cpaulyz deleted the IOTDB-4335 branch September 19, 2022 01:40
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