Обсуждение: the patch: support for DESC/NULLS FIRST/NULLS LAST
Patch for TODO item: - Add support for DESC/NULLS FIRST/NULLS LAST when creating indexes. I had tested it in Windows XP with db 8.2.11 and 8.3.5 Review it, please. ----------------------------------------------- Quan Zongliang quanzongliang@gmail.com CIT Japan: http://www.cit.co.jp
Вложения
Quan Zongliang a écrit : > Patch for TODO item: > - Add support for DESC/NULLS FIRST/NULLS LAST when creating indexes. > > I had tested it in Windows XP with db 8.2.11 and 8.3.5 > > Review it, please. > I compiled it on Linux, and tested it with an 8.3.5 release. It works well (create, modify, view, ...) but I don't like the UI. First, I don't like the group of radio buttons. I think it would be better to replace it with a combo box. Second, the grid should have three columns: Column Name, Order, and NULLs Order. Anyway, it'll be a great new feature for pgAdmin. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
> First, I don't like the group of radio buttons. I think it would be > better to replace it with a combo box. I feel the redio boxes is easy to click than combobox. Let's hear suggestions from others before mdify it. > Second, the grid should have three columns: Column Name, Order, and > NULLs Order. Sure, I will modify. ----------------------------------------------- Quan Zongliang quanzongliang@gmail.com CIT Japan: http://www.cit.co.jp
> First, I don't like the group of radio buttons. I think it would be > better to replace it with a combo box. > > Second, the grid should have three columns: Column Name, Order, and > NULLs Order. All done. Modify mode have one column, new mode have three columns. It's ok? ----------------------------------------------- Quan Zongliang quanzongliang@gmail.com CIT Japan: http://www.cit.co.jp
On Mon, Nov 17, 2008 at 7:25 AM, Quan Zongliang <quanzongliang@gmail.com> wrote: >> First, I don't like the group of radio buttons. I think it would be >> better to replace it with a combo box. I actually think the radio buttons are most appropriate. We normally use a combo box for situations where the values may change in content or quantity which is not the case here. We already do this on the parameters tab of dlgFunction (though the buttons aren't in a panel on there, and the index dialog should remain consistent with that). Other minor issues: The case of the text should follow existing cases as well - if a label is exactly what would be used in an SQL statement, then it's in upper case, otherwise, mixed. EG. DESC, FIRST and LAST, with NULLs and Default. The DESC checkbox doesn't align horizontally with the combo box to its left. The columns list view doesn't size correctly when the form is opened on Mac. This may be because Guillaume's latest sizing fixes haven't been committed yet, or they may need to be extended to cover this dialog. >> Second, the grid should have three columns: Column Name, Order, and >> NULLs Order. > > All done. > > Modify mode have one column, new mode have three columns. It's ok? They should be the same for consistency. One day we may be able to add a column to an index. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
> The case of the text should follow existing cases as well - if a label
In fact, NULLS order's default value is different base of DESC.
When create index:
If DESC is specified, default is NULLS FIRST.
If not (ASC, default), default is NULLS LAST.
So, I think if there is only FIRST and LAST. Maybe it confuse user.
Suggest me, pls.
> The DESC checkbox doesn't align horizontally with the combo box to its left.
Like this: ?
Column <column's combobox>
DESC <checkbox>
NULLS <Default FIRST LAST>
Add Remove
> The columns list view doesn't size correctly when the form is opened
> on Mac
Yes, new method dlgIndex::OnChangeSize() is needed.
I have not Mac, can't test.
PS:
About TODO item:
Allow Sequence to be attached to existing int4/int8 columns
It means to make sequence dialog can support the "OWNED BY" clause in
"CREATE/ALTER SEQUENCE" statement?
-----------------------------------------------
Quan Zongliang
quanzongliang@gmail.com
CIT Japan: http://www.cit.co.jp
On Mon, Nov 17, 2008 at 10:00 AM, Quan Zongliang <quanzongliang@gmail.com> wrote: >> The case of the text should follow existing cases as well - if a label > In fact, NULLS order's default value is different base of DESC. > When create index: > If DESC is specified, default is NULLS FIRST. > If not (ASC, default), default is NULLS LAST. > > So, I think if there is only FIRST and LAST. Maybe it confuse user. > > Suggest me, pls. That's a good point. I think we should remove the default option, so the user can choose explicitly - but, if the user (un)chooses DESC, then the appropriate default should be automatically selected. >> The DESC checkbox doesn't align horizontally with the combo box to its left. > > Like this: ? > Column <column's combobox> > DESC <checkbox> > NULLS <Default FIRST LAST> > Add Remove That would work. At the moment, the DESCK checkbox is a few pixels too low which looks odd. >> The columns list view doesn't size correctly when the form is opened >> on Mac > Yes, new method dlgIndex::OnChangeSize() is needed. > I have not Mac, can't test. No problem. Guillaume and I have Macs and one of us can help out. > PS: > About TODO item: > Allow Sequence to be attached to existing int4/int8 columns > > It means to make sequence dialog can support the "OWNED BY" clause in > "CREATE/ALTER SEQUENCE" statement? That's part of it. I think the TODO item is intended as something that will be done on dlgColumn though. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
> > Allow Sequence to be attached to existing int4/int8 columns > > > > It means to make sequence dialog can support the "OWNED BY" clause in > > "CREATE/ALTER SEQUENCE" statement? > > That's part of it. I think the TODO item is intended as something that > will be done on dlgColumn though. CREATE/ALTER TABLE statement don't support sequence clause, except that column's default value. To support OWNED BY on dlgColumn, I think it is not needed. First, I try to implement it in dlgSequence. It seems simpler than dlgColumn. Ok? Or search the TODO list to find the other I can do. ----------------------------------------------- Quan Zongliang quanzongliang@gmail.com CIT Japan: http://www.cit.co.jp CIT China: http://www.cit.com.cn
On Tue, Nov 18, 2008 at 4:41 AM, Quan Zongliang <quanzongliang@gmail.com> wrote: >> > Allow Sequence to be attached to existing int4/int8 columns >> > >> > It means to make sequence dialog can support the "OWNED BY" clause in >> > "CREATE/ALTER SEQUENCE" statement? >> >> That's part of it. I think the TODO item is intended as something that >> will be done on dlgColumn though. > > CREATE/ALTER TABLE statement don't support sequence clause, except that column's > default value. To support OWNED BY on dlgColumn, I think it is not > needed. > > First, I try to implement it in dlgSequence. > It seems simpler than dlgColumn. > Ok? The point is to turn an existing column into a serial column. In dlgSequence you can specify the dependency, but that doesn't help with the default value of the exiting column. The only logical place to do this is on dlgColumn, and that would require at least 2 SQL queries - one to change the column default, and one to add the dependency link (which is not a problem - we do that all over the place). However, I'm not convinced this feature is worth the effort. I don't think it would be used very much at all. > Or search the TODO list to find the other I can do. There are plenty of other interesting things there :-) -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com