[IOTDB-1931] Adapt tree structrued Template with MManager by bigreybear · Pull Request #4391 · apache/iotdb

bigreybear · 2021-11-15T13:20:40Z

This PR has been merged with new_vector branch in apache repository.
All CI in metadata package passed, while some other packages remaining to be fixed as new_vector branch.

…_vector

…ctorMeasurementSchema in metadata module (apache#4255)

…che#4268)

…_vector

…aths (apache#4283)

…_vector

MarcosZyk · 2021-11-16T02:04:38Z

server/src/main/java/org/apache/iotdb/db/metadata/mtree/traverser/Traverser.java

+    if (!(par instanceof IEntityMNode)) {
+      throw new MetadataException("Measurement not under entity: " + currentNode.getName());
+    }


Avoid instanceof. Use IMNode.isEntity instead.

actually it is not necessary, will be removed then

MarcosZyk · 2021-11-16T02:06:01Z

server/src/main/java/org/apache/iotdb/db/metadata/mtree/traverser/Traverser.java

+    if (!(par instanceof IEntityMNode)) {
+      throw new MetadataException("Measurement not under entity: " + currentNode.getName());
+    }


Is this necessary? How could the parent MNode of current MeasurementMNode not be Entity?

removed now

MarcosZyk · 2021-11-16T02:07:02Z

server/src/main/java/org/apache/iotdb/db/metadata/mtree/traverser/Traverser.java

+    retPath.setUnderAlignedEntity(((IEntityMNode) par).isAligned());
+    return retPath;


Replace (IEntityMNode) par with par.getAsEntity

MarcosZyk · 2021-11-16T02:09:53Z

server/src/main/java/org/apache/iotdb/db/metadata/mtree/traverser/Traverser.java

+      traverseContext.push(node);
      traverse(child, idx + 1, level + 1);
+      traverseContext.pop();
    }


If you want to make the context contains node from root to current node's parent, maybe put this push and pop out of the for loop is better?

all push and pop are out of loop now

MarcosZyk · 2021-11-16T02:27:13Z

server/src/main/java/org/apache/iotdb/db/metadata/MManager.java

+   * Before to insert, set using template if template accessed. Better performance if refactored
+   * with getDeviceNodeWithAutoCreate
+   */
+  private void activateTemplateBeforeAccess(PartialPath deviceId, String[] measurements)
+      throws MetadataException {
+    StringBuilder builder = new StringBuilder(deviceId.getFullPath());
+    if (builder.length() != 0) {
+      builder.append(TsFileConstant.PATH_SEPARATOR);
+    }
+    for (String measurement : measurements) {
+      builder.append(measurement);
+      IMNode mountedNode = mtree.getTemplateMountedNode(new PartialPath(builder.toString()));
+      if (mountedNode != null && !mountedNode.isUseTemplate()) {
+        setUsingSchemaTemplate(mountedNode);
+      }
+      builder.delete(builder.length() - measurement.length(), builder.length());
+    }
+  }


If the mountedNode has not been created on MTree, how could you activate it?

Thanks for you insight!
Now, I will get the template mounted node index, rather than IMNode, so that auto create process methods are decoupled from mounted node detecting.

MarcosZyk · 2021-11-16T02:30:52Z

server/src/main/java/org/apache/iotdb/db/metadata/MManager.java

+    activateTemplateBeforeAccess(devicePath, measurementList);
    IMNode deviceMNode = getDeviceNodeWithAutoCreate(devicePath);


If the device now is represented by template, this method maybe invalid.

As fixed above, this methods no longer exists.

SilverNarcissus

Nice work! Only one format issue and one question. Please have a look~

SilverNarcissus · 2021-11-16T14:27:41Z

server/src/test/java/org/apache/iotdb/db/metadata/MManagerBasicTest.java

    for (MeasurementPath measurementPath :
-        manager.getMeasurementPaths(new PartialPath("root.sg1.d1.*"))) {
-      allSchema.remove(measurementPath.getMeasurementSchema());
+        manager.getMeasurementPaths(new PartialPath("root.sg1.d1.**"))) {


What's the ** means currently? Maybe I miss something in the design part.

Sorry for the confusion, the usage of '**' has not change ever, it is still a multil-level wildcard.

The reason to change here, is that template converted to a tree structured one now. As before, this template has 2 schemasName which won't present in measurement path, 's11' and 'vector'. Now measurements of the template contain 2 sublist, which is 's11' and 'vector.s1....s10'.

So, in order to traverse this template, I modify it to '**' as above.

SilverNarcissus · 2021-11-16T15:12:44Z

server/src/main/java/org/apache/iotdb/db/metadata/MManager.java


-    // 1. get device node
+    // 1. get device node, set using template if accessed.
+


remove this empty line

Thanks! It has been removed now.

SilverNarcissus · 2021-11-16T15:19:55Z

server/src/main/java/org/apache/iotdb/db/metadata/mtree/MTree.java

+            try {
+              resultSet.add(getCurrentPartialPath(node));
+            } catch (MetadataException e) {
+              logger.error(e.getMessage());


Should we throw exception again here?

I'm not sure how to deal with this. It is a exception will not happen actually, so I did not throw it again here.
The exception is caused by a construct of ParitialPath where content are nodes during a traverse which should assure to avoid illegal path. But this method override an abstract method which does not throw exception, so this catch did not throw again.

SilverNarcissus · 2021-11-16T15:24:38Z

server/src/main/java/org/apache/iotdb/db/metadata/mtree/MTree.java

+      } else {
+        for (IMNode child : mountedNode.getChildren().values()) {
+          if (child.isMeasurement()) {
+            if (template.getAlignedPrefixSet().contains("")


What's the meaning of empty string here? Maybe we need some comment

Thanks! This string tries to represent alignment of direct measurement of a template. I will add more comment.

coveralls · 2021-11-17T02:38:27Z

Coverage increased (+0.01%) to 66.979% when pulling f8b9dbf on bigreybear:new_vector_with_treeTemplate into d9d8733 on apache:master.

MarcosZyk · 2021-11-18T07:11:13Z

server/src/main/java/org/apache/iotdb/db/metadata/mtree/traverser/Traverser.java

+  protected MeasurementPath getCurrentMeasurementPathInTraverse(IMeasurementMNode currentNode)
+      throws MetadataException {
+    IMNode par = traverseContext.peek();
+
+    Iterator<IMNode> nodes = traverseContext.descendingIterator();
+    StringBuilder builder = new StringBuilder(nodes.next().getName());
+    while (nodes.hasNext()) {
+      builder.append(TsFileConstant.PATH_SEPARATOR);
+      builder.append(nodes.next().getName());
+    }
+    if (builder.length() != 0) {
+      builder.append(TsFileConstant.PATH_SEPARATOR);
+    }
+    builder.append(currentNode.getName());
+    MeasurementPath retPath =
+        new MeasurementPath(new PartialPath(builder.toString()), currentNode.getSchema());
+    retPath.setUnderAlignedEntity(par.getAsEntityMNode().isAligned());
+    return retPath;
+  }


Maybe move this method to MeasurementCollector is better, if this method is only used in that kind of tasks.

It has been moved now

MarcosZyk · 2021-11-18T07:22:09Z

server/src/main/java/org/apache/iotdb/db/metadata/mtree/MTree.java

+            throw new MetadataException(
+                String.format(
+                    "Path [%s] overlaps but not matches template [%s] under node [%s]",
+                    measurementPath.getFullPath(), upperTemplate.getName(), fullPathNodes[index]));


Shall we make this kind exception message as a specific Exception class?

A new exception TemplateImcompatibleException replaces all these usages.

MarcosZyk · 2021-11-18T07:34:28Z

server/src/main/java/org/apache/iotdb/db/metadata/MManager.java

+      // node might be replaced when check with alignment
+      node = mtree.checkTemplateAlignmentWithMountedNode(node, template);
+
      node.setSchemaTemplate(template);


If this node is not using template, why should it be replaced by entityMNode?

That's true. This check should be a part of setUsingTemplate methods rather than this one. Fixed now.

MarcosZyk · 2021-11-18T07:36:40Z

server/src/main/java/org/apache/iotdb/db/metadata/MManager.java

    // this operation may change mtree structure and node type
    // invoke mnode.setUseTemplate is invalid
    IEntityMNode entityMNode = mtree.setToEntity(node);
+
+    // to ensure alignment adapt with former node or template
+    entityMNode.setAligned(
+        node.isEntity()
+            ? node.getAsEntityMNode().isAligned()
+            : node.getUpperTemplate().isDirectAligned());
    entityMNode.setUseTemplate(true);


Currently, a MNode using template is possibly not an entityMNode since the template may not has direct measurement. Thus the node type transform need check first.

Move the field "isUsingTemplate" from EntityMNode to InternalMNode. This may be better to adapt to Tree template.

In this method (setUsingTemplate), it will check alignment and replace it if direct measurement exists now.
To implement this, I add setUseTemplate into Interface IMNode.
And this method will return an IMNode rather than IEntityNode now.

MarcosZyk · 2021-11-18T07:39:21Z

server/src/main/java/org/apache/iotdb/db/metadata/MManager.java

+    int indexRecord = -1;
+    // check every measurement path
+    for (String measurementId : measurementList) {
+      PartialPath fullPath = devicePath.concatNode(measurementId);
+      int index = mtree.getMountedNodeIndexOnMeasurementPath(fullPath);
+      if (index != fullPath.getNodeLength() - 1) {
+        // this measurement is in template, need to assure mounted node exists and set using
+        // template.
+        if (index != indexRecord) {
+          // Without allowing overlap of template and MTree, this block run only once
+          String[] mountedPathNodes = Arrays.copyOfRange(fullPath.getNodes(), 0, index + 1);
+          IMNode mountedNode = getDeviceNodeWithAutoCreate(new PartialPath(mountedPathNodes));
+          setUsingSchemaTemplate(mountedNode);
+          indexRecord = index;
+        }
+      }
+    }


Since insertPlan only process one device, it is unnecessary to finish the whole loop. Break as early as the device has been checked.

Since it needs to check each measurementPath with template whether compatible, I use a boolean to avoid reduntant process.

SilverNarcissus

Only one issue in IT. Other part are look good to me~

SilverNarcissus · 2021-11-18T08:20:08Z

session/src/test/java/org/apache/iotdb/session/IoTDBSessionSimpleIT.java

+
+    // delete series inside template
+    try {
+      session.deleteTimeseries("root.sg.loc1.sector.x");


add a fail() after this line

Thanks! And it has been added now.

…ssion, modify docs to adapt it

JackieTien97 and others added 30 commits October 28, 2021 11:20

add some interfaces in IWritableMemChunk for flushing

6ffb37c

implement VectorWritableMemChunk

b98ad1f

add another function

f1c0821

Merge branch 'new_vector' of https://github.com/apache/iotdb into new…

66ec32c

…_vector

change some interface

8adf766

refactor memtable write interface

bb0865a

Merge branch 'new_vector' of https://github.com/apache/iotdb into new…

1347328

…_vector

Fix compile

baa8d66

reduce more usage of ifisAligned

0adae7e

support insert vector s1s2s3 then s2s1s3

56cdef7

[To new_vector] Refactor aligned timeseries creation and eliminate Ve…

b1b5ea6

…ctorMeasurementSchema in metadata module (apache#4255)

add VectorSeriesReader

c95efc9

Fix mem control

683b3b4

fix insert bug

843bcd1

[To new_vector] Add MeasurementPath and Refactor groupVectorPath (apa…

27ff250

…che#4268)

resolve conflicts

ae1aab9

add some comments

d418416

fix more insert bugs

8d6985e

Merge branch 'new_vector' of https://github.com/apache/iotdb into new…

8cb77ff

…_vector

spotless

38b81bc

[To new_vector] Eliminate getExactFullPath and refactor groupAlignedP…

6b10062

…aths (apache#4283)

delete useless code line

17ca413

remove more if type=vector

f0e4a8e

change query interface

9f2cf3e

Merge branch 'new_vector' of https://github.com/apache/iotdb into new…

92504cc

…_vector

refacter get readonlymemchunk

1bfa4e0

Fix check path

6678c4a

fix release flushed tvlist npe

db347ba

Fix query vector select ** from root

50fb89c

remove if instance of

b763e96

MarcosZyk reviewed Nov 16, 2021

View reviewed changes

bigreybear added 2 commits November 16, 2021 16:48

fix reviewer's suggestions

bd96fd5

add more test case about template

53fbd03

SilverNarcissus reviewed Nov 16, 2021

View reviewed changes

qiaojialin changed the base branch from new_vector to master November 17, 2021 01:24

bigreybear added 3 commits November 17, 2021 09:40

fix reviewer's suggestions

834bb3a

merge master

06d5b8d

format code with spotless

248d247

bigreybear added 3 commits November 17, 2021 11:04

merge from master

ded2b3a

merge from new master and format code

4e92f30

add IT about Template

0fac301

MarcosZyk reviewed Nov 18, 2021

View reviewed changes

SilverNarcissus approved these changes Nov 18, 2021

View reviewed changes

fix all reviewers suggestion

b7930cd

MarcosZyk approved these changes Nov 18, 2021

View reviewed changes

bigreybear added 4 commits November 18, 2021 23:23

fixed reviews opinion

afd1b27

remove Set<> alignedPrefix from Template, format code with spotless

11b86e4

add more test case

6d5d384

add test case for empty template, remove deprecated interface from se…

8bd5d62

…ssion, modify docs to adapt it

bigreybear force-pushed the new_vector_with_treeTemplate branch from 5a38e3f to 6d5d384 Compare November 20, 2021 01:53

bigreybear added 2 commits November 21, 2021 17:05

Merge branch 'master' into template_adapt

1586cc4

fix bug in MetaUtils.java:GroupAlignedPaths

f8b9dbf

qiaojialin approved these changes Nov 21, 2021

View reviewed changes

qiaojialin merged commit 441d6b7 into apache:master Nov 21, 2021

liuminghui233 mentioned this pull request Jan 11, 2022

[IOTDB-2298] Refactor SQL of schema template #4775

Merged

bigreybear deleted the new_vector_with_treeTemplate branch July 28, 2022 05:09

		retPath.setUnderAlignedEntity(((IEntityMNode) par).isAligned());
		return retPath;

		activateTemplateBeforeAccess(devicePath, measurementList);
		IMNode deviceMNode = getDeviceNodeWithAutoCreate(devicePath);


		// 1. get device node
		// 1. get device node, set using template if accessed.

Conversation

bigreybear commented Nov 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SilverNarcissus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigreybear Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SilverNarcissus Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigreybear Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigreybear Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SilverNarcissus left a comment

bigreybear Nov 17, 2021 •

edited

Loading

SilverNarcissus Nov 16, 2021 •

edited

Loading

coveralls commented Nov 17, 2021 •

edited

Loading

bigreybear Nov 18, 2021 •

edited

Loading

bigreybear Nov 18, 2021 •

edited

Loading