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

Ranges support #44

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Ranges support #44

wants to merge 3 commits into from

Conversation

Alotor
Copy link
Member

@Alotor Alotor commented Jul 26, 2014

Grails doesn't allow you to map directly IntRange and ObjectRange and ignores them! I couldn't find why it does this.

The only solution i could think of is to wrap the ranges in a custom type.

@lmivan check it out and let me know

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.77%) when pulling 09d1848 on ranges_support into 6b9218a on master.

@ilopmar
Copy link
Collaborator

ilopmar commented Jul 26, 2014

Great work!

I'll try to check it tonight or tomorrow.

@ilopmar
Copy link
Collaborator

ilopmar commented Jul 27, 2014

I've revised the changes and I think they're great! 👍
I've rebased origin/master, cleaned up the imports and add a new commit to add seconds to the timestamp string format.

What about tstzrange? In the dialect there's a commented line with this type and also in RangeType there's a constant for TIMESTAMP_TZ_RANGE. Does it work?
I need to do some searchs when I get a stable internet connection but I've doing some tests with bound and unbound lower and upper ranges and I think is weird:

select int4range(0,100,'()');
 int4range 
-----------
 [1,100)

select numrange(0,100,'()');
 numrange 
----------
 (0,100)


select int4range(0,100,'[]');
 int4range 
-----------
 [0,101)

select numrange(0,100,'[]');
 numrange 
----------
 [0,100]

As you can see, if I use numrange the ranges are created propertly but if I use int4range there're not. If tried to use numrange during the inserts but I cann't cast numrange to int4range. I think we should research more on this to be able to define ranges with different lower and upper bounds.

Do you want to change something or should I merge this into master?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.77%) when pulling 6bb8498 on ranges_support into 6b9218a on master.

@Alotor
Copy link
Member Author

Alotor commented Jul 27, 2014

About the the tstzrange I want to make it available when we have tested the ranges more. I wanted to have a version with "easier" types.

About the range functions. I think they are ok be. What do you mean that they don't work? I think we can safely merge into master to have the new type support but we can wait to create a version whe I can implement the criterias.

@ilopmar
Copy link
Collaborator

ilopmar commented Jul 28, 2014

I think the problem is that you can only define this ranges: [) in a table
if you use int4range but the other ranges: (), (] and [] can't be defined.

On the other hand if you use numrange you can define the four ranges. I
think it should be possible to define the four ranges in a domain class.

Sent from my Nexus 10
El 27/07/2014 21:23, "Alonso Torres" [email protected] escribió:

About the the tstzrange I want to make it available when we have tested
the ranges more. I wanted to have a version with "easier" types.

About the range functions. I think they are ok be. What do you mean that
they don't work? I think we can safely merge into master to have the new
type support but we can wait to create a version whe I can implement the
criterias.


Reply to this email directly or view it on GitHub
#44 (comment)
.

@Alotor
Copy link
Member Author

Alotor commented Jul 28, 2014

No, that's not it.

What happens is that for integers [0,100] = (-1, 100] = [0, 101) so
postgres prefers to store it on the last form (always as [) )

For "general numbers" the property [0,100] is not always equal to [0,101)
that's why it's storing it on the same format as stored.


Alonso Javier Torres

On Mon, Jul 28, 2014 at 11:11 AM, Iván López [email protected]
wrote:

I think the problem is that you can only define this ranges: [) in a table
if you use int4range but the other ranges: (), (] and [] can't be defined.

On the other hand if you use numrange you can define the four ranges. I
think it should be possible to define the four ranges in a domain class.

Sent from my Nexus 10
El 27/07/2014 21:23, "Alonso Torres" [email protected] escribió:

About the the tstzrange I want to make it available when we have tested
the ranges more. I wanted to have a version with "easier" types.

About the range functions. I think they are ok be. What do you mean that
they don't work? I think we can safely merge into master to have the new
type support but we can wait to create a version whe I can implement the
criterias.


Reply to this email directly or view it on GitHub
<
https://github.com/kaleidos/grails-postgresql-extensions/pull/44#issuecomment-50282890>

.


Reply to this email directly or view it on GitHub
#44 (comment)
.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.79%) when pulling f9a579d on ranges_support into cc40563 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.8%) when pulling e31a244 on ranges_support into bbc1cb4 on master.

@bryce-gibson
Copy link

I was just wondering whether there's anything outstanding with this PR? :-)

It would be great functionality to have available :-)

@ilopmar
Copy link
Collaborator

ilopmar commented Nov 17, 2014

We were waiting for the new Grails 2.4.4 because it includes a bugfix that allows us to remove a wrapper object that we've created. I think @Alotor will change this in the following weeks :-P

@Alotor
Copy link
Member Author

Alotor commented Nov 17, 2014

Yeah I'll try to wrap this ASAP.

@bryce-gibson
Copy link

Awesome :-)

Just wanted to check it hadn't been forgotten :-)

Thanks for the awesome plugin :-D

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.

4 participants