Skip to content
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

[IOTDB-330] Improve the reading method of unsequence data #619

Merged
merged 12 commits into from
Dec 13, 2019
Merged

[IOTDB-330] Improve the reading method of unsequence data #619

merged 12 commits into from
Dec 13, 2019

Conversation

samperson1997
Copy link
Contributor

@samperson1997 samperson1997 commented Dec 3, 2019

Improve the reading method of unsequence data:

  • Make UnseqResourceMergeReader implement IBatchReader, and use PriorityMergeReader as a variable instead of extending it.
  • Make PriorityMergeReader not implement IPointReader, which returns Element directly instead of returning TimeValuePair
  • Split TimeValuePair in PriorityMergeReader heap element into time and value

@samperson1997 samperson1997 changed the base branch from master to f_batch_reader December 10, 2019 01:20
@samperson1997 samperson1997 changed the title [IOTDB-330] Improve the reading method of non-overlap unsequence data [IOTDB-330] Improve the reading method of unsequence data Dec 13, 2019
@samperson1997 samperson1997 marked this pull request as ready for review December 13, 2019 00:56
index++;
if (index < metaDataList.size()) {
metaData = metaDataList.get(index);
nextMetaDataStartTime = metaData.getStartTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

What if here index = metaDataList.size() and the last updated nextMetaDataStartTime <= priorityMergeReader.current(), will the while (priorityMergeReader.current().getTime() >= nextMetaDataStartTime) loop forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for your reminding! I have modified the code logic in a better way.

public void close() throws IOException {
while (!heap.isEmpty()) {
Element e = heap.poll();
e.close();
}
}

class Element {
public class Element {

IPointReader reader;
Copy link
Contributor

Choose a reason for hiding this comment

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

From my current understanding, there is a future TODO:
ChunkReader is going to implement only the IBatchReader operator. In other words, there will be no more IPointReader or related methods such as TimeValuePair timeValuePair = reader.next();.

Copy link
Contributor

Choose a reason for hiding this comment

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

The future TODO is out of the scope of this pr. This pr is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I intend to make DiskChunkReader and MemChunkReader only implement IBatchReader in next step.

metaData = metaDataList.get(index++);
if (priorityMergeReader.current().getTime() < metaData.getStartTime()) {
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your quick reaction.
But ... the modified logic senses more strange to me, especially the two places:

  1. The index++ logic makes the last metaData of metaDataList got at line 133 can never be added when its getStartTime() <= priorityMergeReader.current().getTime(), because the program will exit in the next loop at line 129.
  2. the addReaderWithPriority is directly used before applying the if (priorityMergeReader.current().getTime() < metaData.getStartTime()) check.

You can modify or discuss about this later. Take your time :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my fault... I have changed the order, how do you think about the codes now? 😂

@qiaojialin qiaojialin merged commit fbdb238 into apache:f_batch_reader Dec 13, 2019
@samperson1997 samperson1997 deleted the unseq_improve branch December 13, 2019 12:07
qiaojialin pushed a commit that referenced this pull request Dec 26, 2019
* add batch reader interfaces

* modify docs in SeriesReaderWithoutValueFilter

* Add data to TSExecuteStatementResp (#631)

* add fill buffer in EngineDataSetWithoutValueFilter (#646)

* [IOTDB-330] Improve the reading method of unsequence data (#619)

* change UnseqResourceMergeReader to IBatchReader

* Fix bug of "Has not execute query" error when querying (#656)

* Original query process (#653)

* add aggregation reader

* Update SeriesReaderWithoutValueFilter.java

* fix code-coverage stage does not compile service-rpc and some other modules

Co-authored-by: Jackie Tien <JackieTien@foxmail.com>
Co-authored-by: Dawei Liu <atoildw@163.com>
Co-authored-by: Zesong Sun <szs19@mails.tsinghua.edu.cn>
Co-authored-by: Lei Rui <33376433+LeiRui@users.noreply.github.com>
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.

3 participants