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

Add qualifed projections to schemas & update dataset#select calls #373

Merged
merged 1 commit into from
Apr 14, 2020

Conversation

abrthel
Copy link
Contributor

@abrthel abrthel commented Mar 5, 2020

Fixes #370

Aliased attributes are context based and should default to the
defined dataset name when using them in most queries.

This fix prevents attributes from being aliased during finalization
and provides a way to tell attributes they will be used in a
context where their configured alias should override the default
attribute name.

(Note: after review and before the pull request is accepted, ping me and I'll squash these commits)

Copy link
Member

@solnic solnic left a comment

Choose a reason for hiding this comment

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

Thank you! This looks good to me.

ps. @flash-gordon ?

@flash-gordon
Copy link
Member

@solnic I'm OK with that. In general, I think we want to do something about caching sql_expr.

@solnic
Copy link
Member

solnic commented Mar 9, 2020

@abrthel please squash commits and I'll merge this in

@abrthel abrthel force-pushed the fix-attribute-aliases-370 branch from acafb76 to 0262df1 Compare March 9, 2020 16:11
@abrthel
Copy link
Contributor Author

abrthel commented Mar 9, 2020

Not really sure what made those checks fail but commits have now been squashed!

@abrthel
Copy link
Contributor Author

abrthel commented Apr 10, 2020

@solnic Hey, is there anything I should be doing to get this merged?

As far as I can tell the tests failed before the test suite even ran.

@solnic
Copy link
Member

solnic commented Apr 11, 2020

@solnic Hey, is there anything I should be doing to get this merged?

Yes, please rebase because the suite should pass now.

Aliased attributes are context based and should default to the
defined dataset name when using them in most queries.

This fix provides a way to tell attributes they will be used in a
context where their configured alias should override the default
attribute name.

As well this commit addresses:

* Fix qualified function attributes
  Makes function attributes use the new qualified
  projection

* Fix postgres commands /w aliased attributes
  Postgres commands were not returning tuples
  with the configured alias names.
@abrthel abrthel force-pushed the fix-attribute-aliases-370 branch from 0262df1 to d9f2058 Compare April 11, 2020 17:07
@rom-bot
Copy link
Collaborator

rom-bot commented Apr 11, 2020

Codacy Here is an overview of what got changed by this pull request:

Clones added
============
- lib/rom/sql/function.rb  2
         

See the complete overview on Codacy

@abrthel
Copy link
Contributor Author

abrthel commented Apr 12, 2020

@solnic done! 😄

@solnic solnic merged commit 45351d3 into rom-rb:master Apr 14, 2020
@solnic solnic added this to the 3.2.1 milestone Apr 14, 2020
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.

Aliased attributes cause various relation functions to output malformed sql
4 participants