ReadV2 in TsFile #144

xingtanzjr · 2018-01-30T12:32:05Z

A new version of read process for TsFile.

Documents for this PR: https://attachments.tower.im/tower/f275e22596b9485da70a98720a9cba24?filename=TsFile+读流程重构文档.pdf

@Beyyes @MyXOF @mdf369 @RelayAnna @jt2594838

close #145

… f_zjr_read_rebuild_pr

…losed

jt2594838 · 2018-01-31T07:10:53Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/common/EncodedSeriesChunkDescriptor.java

+    }
+
+    private String getUUID() {
+        return filePath + offsetInFile + lengthOfBytes;


offsetInFile和lengthOfBytes都是long，直接连接起来是不是不容易分开，需要考虑加一个分隔符吗？

这个生成方法不准确，比如offsetInFile=101，lengthOfBytes=0和offsetInFile=10，lengthOfBytes=10的情况是一样的
还是判断这三者是否相等吧

OK,加一个分隔符

jt2594838 · 2018-01-31T07:13:56Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/controller/MetadataQuerier.java

+/**
+ * Created by zhangjinrui on 2017/12/25.
+ */
+public interface MetadataQuerier {


这里的Metadata是特指seriesChunk的metadata，对吗

这个Metadata泛指TsFile末尾的metadata，但是这个接口的作用主要是给定一个Path，返回该Path的SeriesChunkMetadataList。当前在TsFile中，实现的是返回一个文件中对应Path的List

jt2594838 · 2018-01-31T07:20:22Z

...main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/controller/MetadataQuerierByFileImpl.java

+        randomAccessFileReader.read(buf, 0, buf.length);
+
+        ByteArrayInputStream metadataInputStream = new ByteArrayInputStream(buf);
+        this.fileMetaData = new TsFileMetaDataConverter().toTsFileMetadata(ReadWriteThriftFormatUtils.readFileMetaData(metadataInputStream));


我对这种new一个对象只为调用它的一个方法的做法表示疑问，或许实现为静态方法是个更好的选择。

@MyXOF 这个类你看一下？如果该类在调用方法的时候，不涉及class级别的变量的话，是可以用static的

之后应该会去掉thrift，这个暂时不改了

jt2594838 · 2018-01-31T07:22:27Z

...main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/controller/MetadataQuerierByFileImpl.java

+        try {
+            return seriesChunkDescriptorCache.get(path);
+        } catch (CacheException e) {
+            throw new IOException("Get SeriesChunkDescriptorList Error.", e);


我注意到这里似乎存在IOException -> CacheException -> IOException的过程，那么中间的这一步转换是否必要？
另外，Exception message中如果带上参数（path），可能会比较助于调试。

确实存在这个转换，我也有点疑问，就是说有没有什么好办法让这个地方更优雅一点。这么设计的主要原因是，Cache作为一个独立的类，他的异常不能是IOException，这个应用场景下会发生这种转换，其他应用场景下，转换可能就是AException -> CacheException -> BException了。

Path加在ErrorMsg里面能更清晰，加一下

jt2594838 · 2018-01-31T07:28:45Z

...main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/controller/MetadataQuerierByFileImpl.java

+        return encodedSeriesChunkDescriptorList;
+    }
+
+    private EncodedSeriesChunkDescriptor generateSeriesChunkDescriptorByMetadata(TimeSeriesChunkMetaData timeSeriesChunkMetaData) {


EncodedSeriesChunkDescriptor为何不直接提供一个基于TimeSeriesChunkMetaData的构造函数？

这个提议不错，当时没考虑到。

目前这么实现的一个原因是，TimeSeriesChunkMetadata里面没有filePath这个属性，为了照顾这个点，所以提供了好几个不同的构造函数，直接传入了对应变量的值。

jt2594838 · 2018-01-31T07:31:47Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/controller/SeriesChunkLoader.java

+ * Created by zhangjinrui on 2017/12/26.
+ */
+public interface SeriesChunkLoader {
+    SeriesChunk getMemSeriesChunk(EncodedSeriesChunkDescriptor encodedSeriesChunkDescriptor) throws IOException;


函数名中的Mem是指什么？是指返回对象将被放到内存中还是函数将从内存中获取这个对象？

+1，我在前面也对这个命名有点疑问

MemSeriesChunk是指被加载到内存中的SeriesChunk，即对应SeriesChunk的所有bytes被加载到内存，并且存储在该对象中

jt2594838 · 2018-01-31T07:39:02Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/controller/SeriesChunkLoaderImpl.java

+        byte[] buf = new byte[seriesChunkLength];
+        randomAccessFileReader.seek(encodedSeriesChunkDescriptor.getOffsetInFile());
+        int readLength = randomAccessFileReader.read(buf, 0, seriesChunkLength);
+        if (readLength != seriesChunkLength) {


我似乎看到有些地方做了这个检查，有些地方没有。

根据之前的经验，这个地方是要加这个判断的，因为有时候系统读出来的字节数会有问题（当年遇到过这个坑）

jt2594838 · 2018-01-31T07:40:32Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/datatype/TimeValuePair.java

@@ -42,4 +44,8 @@ public boolean equals(Object object) {
        }
        return false;
    }
+
+    public int getSize() {
+        return 4 + 8 + value.getSize();


这个4指的是？

估计是我弄错了，对象引用是几个字节来着？

不开启引用压缩的情况下是8个，开启引用压缩的情况（大部分）是4个。我不清楚我们有没有开启引用压缩。开启引用压缩会造成一定的性能下降。

这里有结论吗？

jt2594838 · 2018-01-31T07:54:54Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/query/QueryExpression.java

+public class QueryExpression {
+    private List<Path> selectedSeries;
+    private QueryFilter queryFilter;
+    private boolean hasQueryFilter;


hasQueryFilter和queryFilter == null是等价的吗？

理论是上，但是queryFilter == null这是一种潜在的约定，感觉不是很规范。加了一个hasQueryFIlter()方法会更规范，假如某一天修改了hasQueryFIlter()不会对使用该类的其他类产生未知的影响。

jt2594838 · 2018-01-31T08:04:46Z

...du/tsinghua/tsfile/timeseries/readV2/query/impl/QueryDataSetForQueryWithQueryFilterImpl.java

+/**
+ * Created by zhangjinrui on 2017/12/26.
+ */
+public class QueryDataSetForQueryWithQueryFilterImpl implements QueryDataSet {


很想吐槽这个名字。。。

说实话我也想吐槽，当时没想出更好的名字，有啥推荐的名字没，比如QueryWithFilterDataSet？

jt2594838 · 2018-01-31T08:14:36Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/query/impl/QueryExecutorRouter.java

+                if (regularQueryFilter instanceof GlobalTimeFilter) {
+                    return new QueryWithGlobalTimeFilterExecutorImpl(seriesChunkLoader, metadataQuerier).execute(queryExpression);
+                } else {
+                    return new QueryWithQueryFilterExecutorImpl(seriesChunkLoader, metadataQuerier).execute(queryExpression);


同样地，一个对象被创建仅仅是为了调用它的一个方法。
也许把seriesChunkLoader, metadataQuerier，queryExpression合起来做成一个静态函数可以达到同样的效果。

有道理。

这个地方这么设计的一个原因是，没有把excutor看做是一个执行引擎，而是一个“执行者”这样的概念，它其实是可以创建一次，执行多次的，而且内部可能需要维护一些当前执行状态的属性，只不过在这个地方都没有用到

jt2594838 · 2018-01-31T12:36:43Z

...java/cn/edu/tsinghua/tsfile/timeseries/readV2/TsFileGeneratorForSeriesReaderByTimestamp.java

+        //LOG.info(jsonSchema.toString());
+        FileSchema schema = new FileSchema(jsonSchema);
+        TSFileDescriptor.getInstance().getConfig().groupSizeInByte = rowGroupSize;
+        TSFileDescriptor.getInstance().getConfig().maxNumberOfPointsInPage = pageSize;


这个配置项似乎没有改回去，不会对其他测试造成影响吗？

我改一下

jt2594838 · 2018-01-31T13:04:20Z

...ava/cn/edu/tsinghua/tsfile/timeseries/readV2/query/timegenerator/TimestampGeneratorTest.java

+        while (timestampGenerator.hasNext()) {
+//            System.out.println(timestampGenerator.next());
+            Assert.assertEquals(startTimestamp, timestampGenerator.next());
+            startTimestamp += 1;


不需要验证startTimestamp的结束值吗？

对，需要验证一下，不然玩意hasNext直接返回false，UT测不出来

MyXOF · 2018-02-01T02:38:55Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/common/EncodedSeriesChunkDescriptor.java

+    }
+
+    private String getUUID() {
+        return filePath + offsetInFile + lengthOfBytes;


这个生成方法不准确，比如offsetInFile=101，lengthOfBytes=0和offsetInFile=10，lengthOfBytes=10的情况是一样的
还是判断这三者是否相等吧

MyXOF · 2018-02-01T02:40:41Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/query/QueryExpression.java

+        hasQueryFilter = false;
+    }
+
+    public static QueryExpression create() {


private的构造函数，static的构造方法有必要吗？

一种创建表达式的写法风格...感觉应该还好

MyXOF · 2018-02-01T03:26:10Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/query/timegenerator/node/OrNode.java

+                cachedLeftValue = leftValue;
+                return rightValue;
+            } else {
+                return leftValue;


这里二者相等的话，是不是也要向上面那样，hasCachedLeftValue和hasCachedRightValue都设为true

这个逻辑不是这样的，cache某个值的意思是这个值已经被拿出来了，但是还没弄到它。当left==right的时候，说明两个时间戳相等，需要合并为一个时间戳，返回之后谁都不需要被cache

MyXOF · 2018-02-01T03:46:11Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/query/impl/MergeQueryDataSet.java

+
+        @Override
+        public int compareTo(Point o) {
+            return timestamp - o.timestamp > 0 ? 1 : timestamp - o.timestamp < 0 ? -1 : 0;


这我看晕了

MyXOF · 2018-02-01T04:14:57Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/reader/impl/SeriesChunkReader.java

+        throw new IOException("No more timeValuePair in current MemSeriesChunk");
+    }
+
+    private boolean constructPageReaderIfNextSatisfiedPageExists() throws IOException {


这个函数名字感觉把注释要写的内容也包含进去了

哈哈哈哈，可以商讨

Beyyes · 2018-01-31T08:38:51Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/query/timegenerator/node/Node.java

+
+/**
+ * @author Jinrui Zhang
+ */


Add some java doc for this class?
By the way, I think it's not easy to understand these class without documents

这个地方确实需要文档，我后面补一下比较详细的

Beyyes · 2018-02-01T08:09:08Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/basis/ReadOnlyTsFile.java

+public class ReadOnlyTsFile {
+
+    private ITsRandomAccessFileReader randomAccessFileReader;
+    private MetadataQuerier metadataQuerier;


these two variables could be converted to local variables

Querier can be renamed to reader

@Beyyes 啥意思？
@qiaojialin Comment Reject.

该问题忽略

Beyyes · 2018-02-01T08:14:38Z

其实还有个较大的问题，你现在QueryDataSet返回的内容RowRecord，应该没有考虑空值的情况？在IoTDB的group by语句里，其实是有空值的情况的，感觉可以在QueryDataSet里保留一下这个处理逻辑。~！

这个暂时先不处理，考虑以后构造一个更加抽象的能够容纳各种查询结果的QueryDataSet，以用在IoTDB中

Beyyes · 2018-02-01T08:18:29Z

...tsinghua/tsfile/timeseries/readV2/reader/impl/SeriesReaderFromSingleFileByTimestampImpl.java

+        nextSeriesChunkIndex = 0;
+    }
+
+    public SeriesReaderFromSingleFileByTimestampImpl(ITsRandomAccessFileReader randomAccessFileReader, Path path) throws IOException {


why this method is not used?

IoTDB里面用的...

Beyyes · 2018-02-01T08:22:11Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/common/EncodedSeriesChunkDescriptor.java

+ * Created by zhangjinrui on 2017/12/25.
+ */
+public class EncodedSeriesChunkDescriptor implements SeriesChunkDescriptor {
+    private String filePath;


Is the other aggregation values also needed? such as max value, min value, sum, mean?

有道理，这个后面根据情况加一下

Beyyes · 2018-02-01T08:31:54Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/common/MemSeriesChunk.java

+/**
+ * Created by zhangjinrui on 2017/12/25.
+ */
+public class MemSeriesChunk implements SeriesChunk{


I think the naming of this class is not appropriate? Memroy Serius Chunk? What's the meaning?

SeriesChunk本来是指在TsFile（磁盘上的文件）中的概念，这里表示被加载到内存中的SeriesChunk

Beyyes · 2018-02-01T08:53:24Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/reader/impl/PageReader.java

+ */
+public class PageReader implements TimeValuePairReader {
+
+    private TSDataType dataType;


PageReader每次都读一个点，从磁盘IO的角度（读多列，每列都会是随机io）来考虑，是不是效率会比一次把page都读出来效率低一些？

每次只解析一个点，但是Page的所有字节是一次性从磁盘上加载进来的

…ction

… f_zjr_read_rebuild_pr

MyXOF · 2018-02-05T09:07:37Z

包名里面有V2的这次需要去掉吗

MyXOF · 2018-02-05T09:10:44Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/write/page/PageWriterImpl.java

    private void resetTimeStamp() {
        if (totalValueCount == 0)
            minTimestamp = -1;
    }

    @Override
    public void writeToFileWriter(TsFileIOWriter writer, Statistics<?> statistics) throws IOException {
+    	if(minTimestamp==-1){
+    		LOG.error("Write page error, {}, minTime:{}, maxTime:{}",desc,minTimestamp,maxTimestamp);


这里如果出错了，还继续执行下去？

MyXOF

最后几处意见

MyXOF · 2018-02-05T09:11:22Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/write/page/PageWriterImpl.java

@@ -44,6 +44,9 @@ public void writePage(ListByteArrayOutputStream listByteArray, int valueCount, S
        // compress the input data
        if (this.minTimestamp == -1)
            this.minTimestamp = minTimestamp;
+        if(this.minTimestamp==-1){
+        	LOG.error("Write page error, {}, minTime:{}, maxTime:{}",desc,minTimestamp,maxTimestamp);


这里如果出错了，还继续执行下去？

需要，这个是当时测试出来的错误，如果出问题想用error日志记录下来。

MyXOF · 2018-02-05T09:12:02Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/write/io/TsFileIOWriter.java

+
+	public void startRowGroup(String deltaObjectId) {
+		LOG.debug("start row group:{}", deltaObjectId);
+		currentRowGroupMetaData = new RowGroupMetaData(deltaObjectId, 0, 0, new ArrayList<>(), "");// FIXME


// FIXME
是什么意思？

我也不知道以前就有，删除了

MyXOF · 2018-02-05T09:15:32Z

src/main/java/cn/edu/tsinghua/tsfile/timeseries/readV2/reader/impl/PageReader.java

+                return new TsInt(valueDecoder.readInt(valueInputStream));
+            case INT96:
+            case FIXED_LEN_BYTE_ARRAY:
+            case BIGDECIMAL:


这边加一个default？

jt2594838 · 2018-02-05T09:43:11Z

没有别的意见。

… f_zjr_read_rebuild_pr

xingtanzjr and others added 21 commits January 14, 2018 16:28

TMP save 01-10

44d26bb

TMP save 01-16

2fee2f0

add getSize() to TimeValuePair

9a2b0b0

modify interface of SeriesChunk

e1dc833

fix a bug in last commit

ef5b406

add close() method to TimeValuePair interface

a9d19e3

add single series reader for one TsFile

a66a86f

add constructor for TsFileIOWriter

fd3fa8e

add Serializable to TimeValuePair class

2026d94

Merge branch 'f_zjr_read_rebuild_pr' of github.com:thulab/tsfile into…

c9e7bde

… f_zjr_read_rebuild_pr

add startRowGroup and endRowGroup function for tsfile

2be7808

add datatype to TsPrimitiveType

720513c

Merge branch 'f_zjr_read_rebuild_pr' of github.com:thulab/tsfile into…

efd1fcf

… f_zjr_read_rebuild_pr

remove unused lines

87ee4e7

fix a bug in SeriesReaderFromSingleFile where the fileStream is not c…

05393db

…losed

add getByType for TsPrimitiveType

59e453d

change log level from info to debug

999b00b

cancel fill tsfile

8784e02

remove cache and query in seieswriteimpl

f2e7723

add clear() function to Cache

d0c1eb1

remove logback.xml

5f9f0bb

jt2594838 reviewed Jan 31, 2018

View reviewed changes

MyXOF requested changes Feb 1, 2018

View reviewed changes

Beyyes reviewed Feb 1, 2018

View reviewed changes

liukun4515 and others added 9 commits February 1, 2018 17:56

add error log in flush timeseries data

8a20462

add error in flush timeseries data

bd77c43

remove device id from FileSchema

aff995d

Remove unused import

3b4399a

remove the checkIsDeltaExist function, add checkIsTimeSeriesExist fun…

ffe2bbd

…ction

remove the get memory function

d39d885

add more constructor to SeriesReader

2c4e109

Merge branch 'f_zjr_read_rebuild_pr' of github.com:thulab/tsfile into…

a079888

… f_zjr_read_rebuild_pr

repair some codes according to comments from Pull Request

dca0ac6

MyXOF reviewed Feb 5, 2018

View reviewed changes

MyXOF requested changes Feb 5, 2018

View reviewed changes

liukun4515 and others added 4 commits February 6, 2018 10:44

modify the code according to the comments in pr

6a65f5c

modify PageReader

bdf10fe

fix some codes according to comments

2b9e7cc

Merge branch 'f_zjr_read_rebuild_pr' of github.com:thulab/tsfile into…

3e5b35e

… f_zjr_read_rebuild_pr

MyXOF approved these changes Mar 1, 2018

View reviewed changes

MyXOF merged commit ed139d6 into master Mar 7, 2018

Beyyes deleted the f_zjr_read_rebuild_pr branch May 16, 2018 07:22

ReadV2 in TsFile #144

ReadV2 in TsFile #144

Conversation

xingtanzjr commented Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jt2594838 Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jt2594838 Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingtanzjr Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jt2594838 Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jt2594838 Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingtanzjr Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beyyes commented Feb 1, 2018 • edited by xingtanzjr Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyXOF commented Feb 5, 2018

Choose a reason for hiding this comment

MyXOF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jt2594838 commented Feb 5, 2018

xingtanzjr commented Jan 30, 2018 •

edited

Loading

jt2594838 Jan 31, 2018 •

edited

Loading

jt2594838 Jan 31, 2018 •

edited

Loading

xingtanzjr Feb 3, 2018 •

edited

Loading

jt2594838 Feb 5, 2018 •

edited

Loading

jt2594838 Jan 31, 2018 •

edited

Loading

xingtanzjr Feb 3, 2018 •

edited

Loading

Beyyes commented Feb 1, 2018 •

edited by xingtanzjr

Loading