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

Introduce operator of window function. #14213

Merged
merged 75 commits into from
Jan 24, 2025
Merged

Introduce operator of window function. #14213

merged 75 commits into from
Jan 24, 2025

Conversation

Sh-Zh-7
Copy link
Contributor

@Sh-Zh-7 Sh-Zh-7 commented Nov 26, 2024

This PR introduces a window operator in IoTDB, which can be used as backend of window function in SQL.

@Sh-Zh-7 Sh-Zh-7 marked this pull request as draft November 27, 2024 01:25
@Beyyes Beyyes self-requested a review January 2, 2025 09:14
accumulator.evaluateFinal(columnBuilder);
}

public void processStatistics(Statistics[] statistics) {
Copy link
Member

Choose a reason for hiding this comment

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

Can WindowAggregator use statistics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theoretically yes, but it's really rare. A frame must not be smaller than a chunk/page/tsfile so that it can use its statistics.

So I choose to remain this method.

}

public boolean getBoolean(int position) {
ColumnListIndex columnListIndex = getColumnIndex(position);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ColumnListIndex columnListIndex = getColumnIndex(position);
ColumnListIndex columnListIndex = getColumnIndex(position);
int columnIndex = columnListIndex.getColumnIndex();

These two lines are duplicated in getXXX method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two getColumnIndex are different methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe getColumnListIndex is more intuitive.

// Then it is stored in double
private final double startOffset; // For PRECEDING and FOLLOWING use
private final FrameBoundType endType;
private final double endOffset; // Same as startOffset
Copy link
Member

Choose a reason for hiding this comment

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

I see all types of xxxOffset are int, if it's needed to use double?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RangeFrame may have offset with decimal, it cannot be stored by int type.

And the FrameInfo instances count is equal to the number of frame clause in user's SQL, so it wont cause too much extra memory footprint.


// May return null if builder is not full
return transform();
} else if (!cachedTsBlocks.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

consider memory control of cachedBlocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Support for explain analysis is added as well.

// PartitionExecutor only hold reference to TsBlock
// So only cached TsBlocks are considered
long maxPeekMemoryFromCurrent =
(long) cachedTsBlocks.size()
Copy link
Member

Choose a reason for hiding this comment

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

Method calculateMaxPeekMemory is used before the execution of operator, so cachedTsBlocks.size() will always equals to 0.
And the memory of cachedTsBlocks haven been managed by memoryReservationManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

long reserved = tsBlock.getTotalInstanceSize();
memoryReservationManager.reserveMemoryCumulatively(reserved);
totalMemorySize += reserved;
operatorContext.recordSpecifiedInfo(CURRENT_USED_MEMORY, Long.toString(totalMemorySize));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's enough for only recording the max_used_memory.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Sh-Zh-7 Sh-Zh-7 requested a review from Beyyes January 24, 2025 11:16
@Beyyes Beyyes merged commit f2d24ff into apache:master Jan 24, 2025
34 of 35 checks passed
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.

2 participants