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

hstream-sql: code clean up #1488

Merged
merged 7 commits into from
Jul 13, 2023
Merged

hstream-sql: code clean up #1488

merged 7 commits into from
Jul 13, 2023

Conversation

alissa-tung
Copy link
Contributor

PR Description

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation updates required

Summary of the change and which issue is fixed

Main changes: TODO


Checklist

  • I have run format.sh under script
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes

@alissa-tung
Copy link
Contributor Author

I had already detected some bugs and some point to fix or inspect, let me push them further...

@alissa-tung
Copy link
Contributor Author

left a undefined here cc @Commelina 690e594

@@ -37,8 +37,9 @@ flag hstream_use_v2_engine

common shared-properties
ghc-options:
-Wall -Wcompat -Widentities -Wincomplete-record-updates
-Wincomplete-uni-patterns -Wpartial-fields -Wredundant-constraints
-Wall -Wextra -Werror -Wno-name-shadowing -Wno-orphans -Wcompat
Copy link
Member

Choose a reason for hiding this comment

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

Why turn off these warnings(e.g., name-shadowing) at the package level? Prefer to turn it off at the module level if you really want.

@Commelina
Copy link
Contributor

left a undefined here cc @Commelina 690e594

Yes, it is acceptable. BTW, this type (window in tableref and group) is very delicate and misleading, and it will be refactored in the next version.
Also, I noticed that binaryAggOpOnValue has been set to undefined for a long time...

@alissa-tung alissa-tung requested a review from 4eUeP July 13, 2023 03:27
@Commelina
Copy link
Contributor

Would you like to merge it now, or do you have more code to commit...? @alissa-tung

@alissa-tung
Copy link
Contributor Author

i think there will not be more commits

Would you like to merge it now, or do you have more code to commit...? @alissa-tung

@Commelina
Copy link
Contributor

i think there will not be more commits

Would you like to merge it now, or do you have more code to commit...? @alissa-tung

OK. It can be merged after passing checks.

@Commelina Commelina merged commit 53b1dc0 into hstreamdb:main Jul 13, 2023
10 checks passed
@alissa-tung alissa-tung deleted the c branch July 13, 2023 11:32
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