[IOTDB-1883] Extension of schema template to tree-structured by bigreybear · Pull Request #4326 · apache/iotdb

bigreybear · 2021-11-04T21:08:57Z

No description provided.

coveralls · 2021-11-04T22:03:51Z

Coverage decreased (-0.2%) to 66.818% when pulling 3e50d4a on bigreybear:tree_template into 25fefb5 on apache:master.

MarcosZyk · 2021-11-05T07:17:16Z

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

+  public Template getTemplate(String templateName) throws MetadataException {
+    try {
+      return templateManager.getTemplate(templateName);
+    } catch (UndefinedTemplateException e) {
+      throw new MetadataException(e);
+    }
+  }


This method maybe unnecessary. MManager could use templateManager.getTemplate directly.

This method has been removed now. TSServiceImpl now should access Templates via MManager methods.

MarcosZyk · 2021-11-05T07:39:47Z

session/src/main/java/org/apache/iotdb/session/template/Template.java

+  public void addToTemplate(Node child) throws StatementExecutionException {
+    if (moutedNode.getChildren().containsKey(child.getName()))
+      throw new StatementExecutionException("Duplicated child of node in template.");
+    moutedNode.getChildren().put(child.getName(), child);
+  }
+
+  public void deleteFromTemplate(String name) throws StatementExecutionException {
+    if (moutedNode.getChildren().containsKey(name)) moutedNode.getChildren().remove(name);
+    else throw new StatementExecutionException("It is not a direct child of the template: " + name);
+  }


wrap if-else body with { }

MarcosZyk · 2021-11-05T07:47:29Z

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java

+    COUNT_MEA,
+    IS_MEA,
+    IS_SERIES,
+    SHOW_MEA


Is MEA the abbreviation of Measurement? I'think it's better to use full name.

fixed with full name

MarcosZyk · 2021-11-05T07:50:30Z

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java

+  private List<String> allSeriesPaths;
+  private Map<String, IMeasurementSchema> schemaMap;


Is allSeriesPaths duplicated since it can be acquired by invoking schemaMap.keySet().

MarcosZyk · 2021-11-05T09:55:12Z

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogTxtWriter.java

+  public void appendTemplate(AppendTemplatePlan plan) throws IOException {
+    StringBuilder buf = new StringBuilder();
+    for (int i = 0; i < plan.getMeasurements().size(); i++) {
+      buf.append(
+          String.format(
+              "%s,%s,%s,%s,%s,%s,%s",
+              MetadataOperationType.APPEND_TEMPLATE,
+              plan.getName(),
+              plan.isAligned() ? 1 : 0,
+              plan.getMeasurements().get(i),
+              plan.getDataTypes().get(i).serialize(),
+              plan.getEncodings().get(i).serialize(),
+              plan.getCompressors().get(i).serialize()));
+      buf.append(LINE_SEPARATOR);
+      lineNumber.incrementAndGet();
+    }
+    ByteBuffer buff = ByteBuffer.wrap(buf.toString().getBytes());
+    channel.write(buff);
+  }
+
+  public void pruneTemplate(PruneTemplatePlan plan) throws IOException {
+    StringBuilder buf = new StringBuilder();
+
+    for (int i = 0; i < plan.getPrunedMeasurements().size(); i++) {
+      buf.append(
+          String.format(
+              "%s,%s,%s,%s",
+              MetadataOperationType.PRUNE_TEMPLATE,
+              plan.getName(),
+              plan.getPrunedMeasurements().get(i),
+              ""));
+      buf.append(LINE_SEPARATOR);
+      lineNumber.incrementAndGet();
+    }
+
+    for (int i = 0; i < plan.getPrunedPrefix().size(); i++) {
+      buf.append(
+          String.format(
+              "%s,%s,%s,%s",
+              MetadataOperationType.PRUNE_TEMPLATE,
+              plan.getName(),
+              "",
+              plan.getPrunedPrefix().get(i)));
+      buf.append(LINE_SEPARATOR);
+      lineNumber.incrementAndGet();
+    }
+
+    ByteBuffer buff = ByteBuffer.wrap(buf.toString().getBytes());
+    channel.write(buff);
+  }
+


MLogTxtReader and MLogTxtWriter is only used for MLogParser. One plan should be presented in one line.

TxtWriter will print only one line for plans about templates now.

MarcosZyk · 2021-11-05T10:02:13Z

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java

+  // region unimplemented interfaces
+  public boolean updateSeriesDataType(String path, TSDataType dataType) {
+    return true;
+  }
+
+  public boolean updateSeriesEncoding(String path, TSEncoding encoding) {
+    return true;
+  }
+
+  public boolean updateSeriesCompression(String path, CompressionType compressor) {
+    return true;
+  }
+
+  public boolean updateAlignedPrefixCompression(String prefix, CompressionType compressor) {
+    return true;
+  }
+
+  public boolean upsertSeries(
+      String path, TSDataType dataType, TSEncoding encoding, CompressionType compressionType) {
+    return true;
+  }
+  // endregion


I'don't think we need to support schema modification. If users want to alter the schema of one timeseries, they should delete target timeseries first and create what they want.

all removed

MarcosZyk · 2021-11-05T10:04:34Z

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java

+  @Deprecated
  public boolean isCompatible(PartialPath path) {
-    return !(schemaMap.containsKey(path.getMeasurement())
-        || schemaMap.containsKey(path.getDevicePath().getMeasurement()));
+    if (mountedNode.hasChild(path.getMeasurement())
+        && mountedNode.getChild(path.getMeasurement()).isMeasurement()) return false;
+    if (mountedNode.hasChild(path.getDevicePath().getMeasurement())
+        && mountedNode.getChild(path.getDevicePath().getMeasurement()).isMeasurement())
+      return false;
+    return true;
  }


If no more usage, delete this method directly.

it's removed and substituted with a new method:
public boolean isDirectNodeInTemplate(String nodeName);

MarcosZyk · 2021-11-05T10:16:05Z

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java

+  private IMNode mountedNode;
+  private Set<String> alignedPrefix;
+  private List<String> mountedPath;


It is designed that one template can be set on one mnode and the descendants of this mnode can use this template, which we call template instantiation.
I think the mountedNode in your code is the mnode this template set to and the mountedPath represents the mnode using this template.
It's better to use different terms to distinguish such two kinds mnodes. Or generating your mountedPath at runtime via a new traverse method in MTree.

renamed to templateRoot now

qiaojialin

update UserGuide in docs/UserGuide

server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java

thrift/src/main/thrift/rpc.thrift

session/src/test/java/org/apache/iotdb/session/template/TemplateUT.java

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java

session/src/main/java/org/apache/iotdb/session/template/Template.java

… reviews fixed

…agaer to query template information

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

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

session/src/test/java/org/apache/iotdb/session/template/TemplateUT.java

qiaojialin · 2021-11-07T09:57:18Z

session/src/main/java/org/apache/iotdb/session/template/Template.java

+
+  // sync with metadata.Template
+  public enum TemplateQueryType {
+    NULL,


what's this for?

session/src/main/java/org/apache/iotdb/session/template/Template.java

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

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java

server/src/main/java/org/apache/iotdb/db/qp/physical/crud/CreateTemplatePlan.java

qiaojialin · 2021-11-07T12:42:11Z

server/src/main/java/org/apache/iotdb/db/service/TSServiceImpl.java

+    List<TSDataType> dataTypes = new ArrayList<>();
+    List<TSEncoding> encodings = new ArrayList<>();
+    List<CompressionType> compressionTypes = new ArrayList<>();


use array TSDatatType[]
Also change the field type in Plan

All plan maintains arrays rather than lists, but getters will return still return lists. Constructors support both list and array parameters.
interfaces in TSServiceImpl uses arrays now.

session/src/main/java/org/apache/iotdb/session/Session.java

SilverNarcissus

Fantastic feature and I'm looking forward using it. There are some issues, please have a look.

SilverNarcissus · 2021-11-08T02:54:37Z

docs/UserGuide/API/Programming-Java-Native-API.md

+                              TSEncoding[] encodings,
+                              CompressionType[] compressors);
+
+// Add one alinged measurement to a template


(1) typo: 'alinged'
(2) we really need add one aligned measurement?

Thanks!
I think may be used to add an aligned measurement to an exsiting parent?

and typo fixed

SilverNarcissus · 2021-11-08T03:01:16Z

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java


+  // region construct template tree
+  /** Construct aligned measurements, checks prefix equality, path duplication and conflict */
+  private void constructTemplateTree(String[] alignedPaths, IMeasurementSchema[] schemas)


This method contains too much if else. Could you simplify it? Or you can add some comments for each path
suggestion: exception case should as front of the method as they can

Thanks and now it is simplified than last commit

SilverNarcissus · 2021-11-08T04:15:31Z

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

+      assertEquals("to", template.getDirectNode("to").getName());
+
+      try {
+        template.deleteMeasurements("a.single");


we should add a fail() after this line

SilverNarcissus · 2021-11-08T04:18:10Z

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

+    assertEquals(
+        "[vehicle.GPS.y, x, vehicle.GPS.x, y, GPS.x, vehicle.x, GPS.y, vehicle.y]",
+        session.showMeasurementsInTemplate("treeTemplate").toString());
+    assertEquals(8, session.countMeasurementsInTemplate("treeTemplate"));


Maybe we should add some insert and select test after change the template

more add and delete now

thrift/src/main/thrift/rpc.thrift

SilverNarcissus

There are failing test. Please have a look.
Please use

mvn spotless:apply

to format your code.

MarcosZyk · 2021-11-08T11:16:37Z

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

+    } catch (UndefinedTemplateException e) {
+      e.printStackTrace();
+    }
+    return -1;


throw exception rather than return -1 or e.printStackTrace()

SilverNarcissus

There are 2 small issues, please have a check.

SilverNarcissus · 2021-11-08T12:01:55Z

session/src/main/java/org/apache/iotdb/session/Session.java

+    defaultSessionConnection.createSchemaTemplate(req);
+  }
+
+  public void addAlignedMeasurementsInTemplate(


public interface should add java doc

java docs about create, prune and query the template has been added now.

SilverNarcissus · 2021-11-08T12:05:23Z

server/src/main/java/org/apache/iotdb/db/metadata/template/Template.java

+  }
+
+  public List<String> getAllMeasurementsPaths() {
+    //    traverse();


remove comment line

its removed now

…seless comment.

SilverNarcissus

LGTM

bigreybear force-pushed the tree_template branch 4 times, most recently from 3f2d824 to 744ec33 Compare November 5, 2021 03:13

MarcosZyk reviewed Nov 5, 2021

View reviewed changes

qiaojialin requested changes Nov 5, 2021

View reviewed changes

bigreybear force-pushed the tree_template branch from 744ec33 to d17a4a8 Compare November 6, 2021 17:04

[IOTDB-1883] Extension of schema template to tree-structured, with PR…

f4c44ef

… reviews fixed

bigreybear force-pushed the tree_template branch from d17a4a8 to f4c44ef Compare November 6, 2021 17:14

bigreybear added 7 commits November 7, 2021 11:20

Schema emplate plan txt log now writes in one line

1c12ac0

Templates only access inside MManager now, TSServiceImp will call MMn…

c0ab1f9

…agaer to query template information

fix code formats

f43dca1

import Pair from tsfile package to solve CI failure

6529cb4

fix CreateSchemaTemplate test in example package

f65e4a6

fix CreateSchemaTemplate at cluster package

359e1fe

Fixed CI failures, except for traverse

ae38792

qiaojialin requested changes Nov 7, 2021

View reviewed changes

SilverNarcissus reviewed Nov 8, 2021

View reviewed changes

bigreybear added 6 commits November 8, 2021 13:54

fix reviews as commented

ed4fa6e

add license to new file

e418078

remove UT about template in traverser

002b95d

spotless

6750567

fix UT with template txt mlog

95d86d3

rename TemplateQueryType.IS_SERIES to TemplateQueryType.PATH_EXIST

8568a05

MarcosZyk reviewed Nov 8, 2021

View reviewed changes

MarcosZyk approved these changes Nov 8, 2021

View reviewed changes

throw exception rather than -1 at counMeasurementsInTemplate

dc3c5a8

SilverNarcissus reviewed Nov 8, 2021

View reviewed changes

add javadoc to interfaces about CRUD of template in session, remove u…

7389a25

…seless comment.

qiaojialin approved these changes Nov 8, 2021

View reviewed changes

SilverNarcissus approved these changes Nov 8, 2021

View reviewed changes

better docs format

3e50d4a

qiaojialin merged commit a3c38e3 into apache:master Nov 8, 2021

liuminghui233 mentioned this pull request Jan 11, 2022

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

Merged

		private List<String> allSeriesPaths;
		private Map<String, IMeasurementSchema> schemaMap;

Conversation

bigreybear commented Nov 4, 2021

Uh oh!

coveralls commented Nov 4, 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!

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!

qiaojialin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 4, 2021 •

edited

Loading