Skip to content

Enable Queries on Imported/Exported Tables#225

Merged
dey4ss merged 8 commits intomasterfrom
deyass/export_select
Feb 7, 2023
Merged

Enable Queries on Imported/Exported Tables#225
dey4ss merged 8 commits intomasterfrom
deyass/export_select

Conversation

@dey4ss
Copy link
Copy Markdown
Member

@dey4ss dey4ss commented Nov 17, 2022

In hyrise/hyrise#2514, we sketched how filtering and encoding selection during import and export of tables does look like in Posgtres [1]. After a discusison in the weekly, this PR enables filters on import/export of tables. The export filter will also be implemented in Hyrise.

[1] https://www.postgresql.org/docs/15/sql-copy.html

@dey4ss dey4ss requested review from Bouncner and klauck November 17, 2022 12:58
Copy link
Copy Markdown
Collaborator

@Bouncner Bouncner left a comment

Choose a reason for hiding this comment

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

I don't have much to say.

Comment thread test/queries/queries-good.sql
@Bouncner
Copy link
Copy Markdown
Collaborator

Fine from my side, but as I am not familiar with the parser, maybe wait for another approval?

@dey4ss dey4ss requested a review from mweisgut November 28, 2022 14:16
Comment thread src/sql/statements.cpp Outdated
Comment thread src/parser/bison_parser.y
// Specify code that is included in the generated .h and .c files
// clang-format off
%code requires {
// clang-format on
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why remove? Does clang complain about the format?

Copy link
Copy Markdown
Member Author

@dey4ss dey4ss Feb 3, 2023

Choose a reason for hiding this comment

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

To be honest, gonna have to look into this after that time. Think that clang-format at that time (probably 14 or so) did some weird stuff then. The clang-format comments don't suppress clang-tidy/linter warnings, but prevent clang-format from reformating the enclosed lines (e.g., indentation, which does not work well in the parser file since it's not really a C++ source file).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed, clang-format (15) locally screws up the indentation and even adds linebreaks in the bison control lines.

Comment thread src/sql/statements.cpp Outdated
Comment thread src/sql/Expr.cpp Outdated
Comment thread src/sql/Expr.cpp Outdated
Copy link
Copy Markdown
Contributor

@klauck klauck left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I had only

  • one question
  • two style questions (but the proposed versions are ok for me)

.

@dey4ss
Copy link
Copy Markdown
Member Author

dey4ss commented Feb 4, 2023

@Bouncner @klauck should be ready to go now. Knaller? 🧨 💥

@Bouncner
Copy link
Copy Markdown
Collaborator

Bouncner commented Feb 4, 2023

@Bouncner @klauck should be ready to go now. Knaller? 🧨 💥

🧨 💥 ❗

@dey4ss
Copy link
Copy Markdown
Member Author

dey4ss commented Feb 4, 2023

Argh, Travis doesn't wanna work again 😡
Claims our credits are completely consumed while we still have lots of them, doesn't even list this branch as a PR. Starting to really hate that s***. Any ideas?

@Bouncner
Copy link
Copy Markdown
Collaborator

Bouncner commented Feb 5, 2023

Mailing TravisCI? :-/

@dey4ss
Copy link
Copy Markdown
Member Author

dey4ss commented Feb 7, 2023

Yeah, already did that. Let's see what happens.

@dey4ss
Copy link
Copy Markdown
Member Author

dey4ss commented Feb 7, 2023

They fixed it.

@dey4ss dey4ss merged commit 5a5fd24 into master Feb 7, 2023
@dey4ss dey4ss deleted the deyass/export_select branch February 7, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants