Commit bdcdaca8 authored by Kazuhiko Shiozaki's avatar Kazuhiko Shiozaki

Add auto_extend_select_list argument in buildSQLQuery() and use alias in...

Add auto_extend_select_list argument in buildSQLQuery() and use alias in group_by_expression and order_by_expression.

If True, select_list is automatically extended to have columns used in
group_by_list and order_by_list. It is useful when use
select_expression in inner query and use group_by_expression or
order_by_expression in outer query.
parent 2b71f08a
...@@ -80,10 +80,10 @@ class TestI18NSearch(ERP5TypeTestCase): ...@@ -80,10 +80,10 @@ class TestI18NSearch(ERP5TypeTestCase):
self.assertEqual(result[0].getPath(), self.person1.getPath()) self.assertEqual(result[0].getPath(), self.person1.getPath())
# check sort on fulltext column # check sort on fulltext column
self.assertFalse('ORDER BY\n MATCH' in self.portal.portal_catalog(SearchableText='Faure', sort_on=(('SearchableText', 'ascending'),), src__=1)) self.assertTrue('ORDER BY\n `full_text`.`SearchableText` ASC' in self.portal.portal_catalog(SearchableText='Faure', sort_on=(('SearchableText', 'ascending'),), src__=1))
# check sort on fulltext search score # check sort on fulltext search score
self.assertTrue('ORDER BY\n MATCH' in self.portal.portal_catalog(SearchableText='Faure', sort_on=(('SearchableText__score__', 'ascending'),), src__=1)) self.assertTrue('ORDER BY\n full_text_SearchableText__score__ ASC' in self.portal.portal_catalog(SearchableText='Faure', sort_on=(('SearchableText__score__', 'ascending'),), src__=1))
def test_catalog_full_text_title(self): def test_catalog_full_text_title(self):
# check if 'é' == 'e' collation works # check if 'é' == 'e' collation works
...@@ -108,18 +108,23 @@ class TestI18NSearch(ERP5TypeTestCase): ...@@ -108,18 +108,23 @@ class TestI18NSearch(ERP5TypeTestCase):
self.assertEqual(result[0].getPath(), self.person3.getPath()) self.assertEqual(result[0].getPath(), self.person3.getPath())
# check sort on fulltext column # check sort on fulltext column
self.assertFalse('ORDER BY\n MATCH' in self.portal.portal_catalog(**{ self.assertFalse('ORDER BY\n catalog_full_text_title__score__ ASC' in self.portal.portal_catalog(**{
'catalog_full_text.title':'Faure', 'catalog_full_text.title':'Faure',
'sort_on':(('catalog_full_text.title', 'ascending'),), 'sort_on':(('catalog_full_text.title', 'ascending'),),
'src__':1 'src__':1
})) }))
# check sort on fulltext search score # check sort on fulltext search score
self.assertFalse('ORDER BY\n MATCH' in self.portal.portal_catalog(**{ self.assertTrue('ORDER BY\n catalog_full_text_title__score__' in self.portal.portal_catalog(**{
'catalog_full_text.title':'Faure', 'catalog_full_text.title':'Faure',
'sort_on':(('catalog_full_text.title__score__', 'ascending'),), 'sort_on':(('catalog_full_text.title__score__', 'ascending'),),
'src__':1 'src__':1
})) }))
self.assertTrue('ORDER BY\n catalog_full_text_title__score__' in self.portal.portal_catalog(**{
'catalog_full_text.title':'Faure',
'sort_on':(('title__score__', 'ascending'),),
'src__':1
}))
@expectedFailure @expectedFailure
def test_full_text_title(self): def test_full_text_title(self):
...@@ -148,10 +153,10 @@ class TestI18NSearch(ERP5TypeTestCase): ...@@ -148,10 +153,10 @@ class TestI18NSearch(ERP5TypeTestCase):
self.assertTrue('MATCH' in self.portal.portal_catalog(destination_title='Faure', src__=1)) self.assertTrue('MATCH' in self.portal.portal_catalog(destination_title='Faure', src__=1))
# check sort on fulltext column # check sort on fulltext column
self.assertFalse('ORDER BY\n MATCH' in self.portal.portal_catalog(title='Faure', sort_on=(('title', 'ascending'),), src__=1)) self.assertTrue('ORDER BY\n `catalog`.`title` ASC' in self.portal.portal_catalog(title='Faure', sort_on=(('title', 'ascending'),), src__=1))
# check sort on fulltext search score # check sort on fulltext search score
self.assertTrue('ORDER BY\n MATCH' in self.portal.portal_catalog(title='Faure', sort_on=(('title__score__', 'ascending'),), src__=1)) self.assertTrue('ORDER BY\n catalog_full_text_title__score__' in self.portal.portal_catalog(title='Faure', sort_on=(('title__score__', 'ascending'),), src__=1))
def test_suite(): def test_suite():
suite = unittest.TestSuite() suite = unittest.TestSuite()
......
...@@ -453,6 +453,9 @@ class ColumnMap(object): ...@@ -453,6 +453,9 @@ class ColumnMap(object):
def asSQLColumn(self, raw_column, group=DEFAULT_GROUP_ID): def asSQLColumn(self, raw_column, group=DEFAULT_GROUP_ID):
if self.catalog_table_name is None or raw_column in self.column_ignore_set or \ if self.catalog_table_name is None or raw_column in self.column_ignore_set or \
'.' in raw_column or '*' in raw_column: '.' in raw_column or '*' in raw_column:
if raw_column.endswith('__score__'):
result = raw_column.replace('.', '_')
else:
result = raw_column result = raw_column
else: else:
if raw_column.endswith('__score__'): if raw_column.endswith('__score__'):
...@@ -464,7 +467,10 @@ class ColumnMap(object): ...@@ -464,7 +467,10 @@ class ColumnMap(object):
if group is DEFAULT_GROUP_ID: if group is DEFAULT_GROUP_ID:
group, column = self.related_key_dict.get(column, (group, raw_column)) group, column = self.related_key_dict.get(column, (group, raw_column))
alias = self.table_alias_dict[(group, self.column_map[(group, column)])] alias = self.table_alias_dict[(group, self.column_map[(group, column)])]
result = '`%s`.`%s%s`' % (alias, column, column_suffix) if column_suffix:
result = '%s_%s%s' % (alias, column, column_suffix)
else:
result = '`%s`.`%s`' % (alias, column)
if function is not None: if function is not None:
result = '%s(%s)' % (function, result) result = '%s(%s)' % (function, result)
return result return result
......
...@@ -116,16 +116,15 @@ class MatchComparisonOperator(MonovaluedComparisonOperator): ...@@ -116,16 +116,15 @@ class MatchComparisonOperator(MonovaluedComparisonOperator):
} }
select_dict = {} select_dict = {}
if not only_group_columns: if not only_group_columns:
select_dict['%s__score__' % column.replace('`', '').rsplit('.', 1)[-1]] = match_string select_dict['%s__score__' % column.replace('`', '').replace('.', '_')] = match_string
# Support sort on the relevance by using (column)__score__ key.
order_by_dict = { order_by_dict = {
'`%s__score__`' % '`.`'.join([x.strip('`') for x in column.split('.')]): match_string, '%s__score__' % column.replace('`', '').replace('.', '_'): match_string
} }
return SQLExpression( return SQLExpression(
self, self,
select_dict=select_dict, select_dict=select_dict,
where_expression=match_string,
order_by_dict=order_by_dict, order_by_dict=order_by_dict,
where_expression=match_string,
can_merge_select_dict=True, can_merge_select_dict=True,
) )
......
...@@ -61,6 +61,7 @@ class EntireQuery(object): ...@@ -61,6 +61,7 @@ class EntireQuery(object):
left_join_list=(), left_join_list=(),
limit=None, limit=None,
catalog_table_name=None, catalog_table_name=None,
auto_extend_select_list=False,
extra_column_list=(), extra_column_list=(),
from_expression=None, from_expression=None,
order_by_override_list=None, order_by_override_list=None,
...@@ -76,6 +77,7 @@ class EntireQuery(object): ...@@ -76,6 +77,7 @@ class EntireQuery(object):
self.extra_column_list = list(extra_column_list) self.extra_column_list = list(extra_column_list)
self.from_expression = from_expression self.from_expression = from_expression
self.implicit_join = implicit_join self.implicit_join = implicit_join
self.auto_extend_select_list = auto_extend_select_list
def asSearchTextExpression(self, sql_catalog): def asSearchTextExpression(self, sql_catalog):
return self.query.asSearchTextExpression(sql_catalog) return self.query.asSearchTextExpression(sql_catalog)
...@@ -211,6 +213,7 @@ class EntireQuery(object): ...@@ -211,6 +213,7 @@ class EntireQuery(object):
order_by_list=self.order_by_list, order_by_list=self.order_by_list,
group_by_list=self.group_by_list, group_by_list=self.group_by_list,
select_dict=self.final_select_dict, select_dict=self.final_select_dict,
auto_extend_select_list=self.auto_extend_select_list,
limit=self.limit, limit=self.limit,
where_expression_operator='and', where_expression_operator='and',
sql_expression_list=self.sql_expression_list) sql_expression_list=self.sql_expression_list)
......
...@@ -2350,7 +2350,8 @@ class Catalog(Folder, ...@@ -2350,7 +2350,8 @@ class Catalog(Folder,
return order_by_list return order_by_list
def buildEntireQuery(self, kw, query_table='catalog', ignore_empty_string=1, def buildEntireQuery(self, kw, query_table='catalog', ignore_empty_string=1,
limit=None, extra_column_list=()): limit=None, auto_extend_select_list=False,
extra_column_list=()):
group_by_list = kw.pop('group_by_list', kw.pop('group_by', kw.pop('group_by_expression', ()))) group_by_list = kw.pop('group_by_list', kw.pop('group_by', kw.pop('group_by_expression', ())))
if isinstance(group_by_list, basestring): if isinstance(group_by_list, basestring):
group_by_list = [x.strip() for x in group_by_list.split(',')] group_by_list = [x.strip() for x in group_by_list.split(',')]
...@@ -2417,18 +2418,21 @@ class Catalog(Folder, ...@@ -2417,18 +2418,21 @@ class Catalog(Folder,
implicit_join=implicit_join, implicit_join=implicit_join,
limit=limit, limit=limit,
catalog_table_name=query_table, catalog_table_name=query_table,
auto_extend_select_list=auto_extend_select_list,
extra_column_list=extra_column_list, extra_column_list=extra_column_list,
from_expression=from_expression) from_expression=from_expression)
def buildSQLQuery(self, query_table='catalog', REQUEST=None, def buildSQLQuery(self, query_table='catalog', REQUEST=None,
ignore_empty_string=1, only_group_columns=False, ignore_empty_string=1, only_group_columns=False,
limit=None, extra_column_list=(), limit=None, auto_extend_select_list=False,
extra_column_list=(),
**kw): **kw):
return self.buildEntireQuery( return self.buildEntireQuery(
kw, kw,
query_table=query_table, query_table=query_table,
ignore_empty_string=ignore_empty_string, ignore_empty_string=ignore_empty_string,
limit=limit, limit=limit,
auto_extend_select_list=auto_extend_select_list,
extra_column_list=extra_column_list, extra_column_list=extra_column_list,
).asSQLExpression( ).asSQLExpression(
self, self,
......
...@@ -94,6 +94,7 @@ class SQLExpression(object): ...@@ -94,6 +94,7 @@ class SQLExpression(object):
where_expression_operator=None, where_expression_operator=None,
sql_expression_list=(), sql_expression_list=(),
select_dict=None, select_dict=None,
auto_extend_select_list=False,
limit=None, limit=None,
from_expression=None, from_expression=None,
can_merge_select_dict=False): can_merge_select_dict=False):
...@@ -120,6 +121,7 @@ class SQLExpression(object): ...@@ -120,6 +121,7 @@ class SQLExpression(object):
sql_expression_list = [x for x in sql_expression_list if x is not None] sql_expression_list = [x for x in sql_expression_list if x is not None]
self.sql_expression_list = list(sql_expression_list) self.sql_expression_list = list(sql_expression_list)
self.select_dict = defaultDict(select_dict) self.select_dict = defaultDict(select_dict)
self.auto_extend_select_list = auto_extend_select_list
if limit is None: if limit is None:
self.limit = () self.limit = ()
elif isinstance(limit, (list, tuple)): elif isinstance(limit, (list, tuple)):
...@@ -133,6 +135,17 @@ class SQLExpression(object): ...@@ -133,6 +135,17 @@ class SQLExpression(object):
warnings.warn("Providing a 'from_expression' is deprecated.", warnings.warn("Providing a 'from_expression' is deprecated.",
DeprecationWarning) DeprecationWarning)
self.from_expression = from_expression self.from_expression = from_expression
self._select_dict = self._getSelectDict()[0]
if self.auto_extend_select_list:
select_column_set = {y for x, y in self._select_dict.iteritems()}
extend_column_set = set(self.group_by_list).union(
{x[0] for x in self.order_by_list})
for i in extend_column_set.difference(select_column_set):
# '__score__' suffix alias is already added in select_dict by
# MatchComparisonOperator.
if '__score__' not in i:
self._select_dict['%s__ext__' % i.replace('`', '').replace('.', '_')] = i
self._reversed_select_dict = {y: x for x, y in self._select_dict.iteritems()}
def getTableAliasDict(self): def getTableAliasDict(self):
""" """
...@@ -236,9 +249,8 @@ class SQLExpression(object): ...@@ -236,9 +249,8 @@ class SQLExpression(object):
append = result.append append = result.append
order_by_dict = self._getOrderByDict() order_by_dict = self._getOrderByDict()
for (column, direction, cast) in self.getOrderByList(): for (column, direction, cast) in self.getOrderByList():
if column.endswith('__score__') and column not in order_by_dict:
continue
expression = conflictSafeGet(order_by_dict, column, str(column)) expression = conflictSafeGet(order_by_dict, column, str(column))
expression = self._reversed_select_dict.get(expression, expression)
if cast not in (None, ''): if cast not in (None, ''):
expression = 'CAST(%s AS %s)' % (expression, cast) expression = 'CAST(%s AS %s)' % (expression, cast)
if direction is not None: if direction is not None:
...@@ -304,7 +316,7 @@ class SQLExpression(object): ...@@ -304,7 +316,7 @@ class SQLExpression(object):
If there are nested SQLExpression, it merges (union of sets) them with If there are nested SQLExpression, it merges (union of sets) them with
local value. local value.
""" """
result = set(self.group_by_list) result = {self._reversed_select_dict.get(x, x) for x in self.group_by_list}
for sql_expression in self.sql_expression_list: for sql_expression in self.sql_expression_list:
result.update(sql_expression.getGroupByset()) result.update(sql_expression.getGroupByset())
return result return result
...@@ -363,7 +375,7 @@ class SQLExpression(object): ...@@ -363,7 +375,7 @@ class SQLExpression(object):
checks that they don't alias different columns with the same name. If checks that they don't alias different columns with the same name. If
they do, it raises a ValueError. they do, it raises a ValueError.
""" """
return self._getSelectDict()[0] return self._select_dict
def getSelectExpression(self): def getSelectExpression(self):
""" """
......
...@@ -45,6 +45,7 @@ class IEntireQuery(Interface): ...@@ -45,6 +45,7 @@ class IEntireQuery(Interface):
def __init__(query, order_by_list=None, group_by_list=None, def __init__(query, order_by_list=None, group_by_list=None,
select_dict=None, limit=None, catalog_table_name=None, select_dict=None, limit=None, catalog_table_name=None,
auto_extend_select_list=False,
extra_column_list=None, from_expression=None, extra_column_list=None, from_expression=None,
order_by_override_list=None): order_by_override_list=None):
""" """
...@@ -67,6 +68,11 @@ class IEntireQuery(Interface): ...@@ -67,6 +68,11 @@ class IEntireQuery(Interface):
See SQLExpression. See SQLExpression.
catalog_table_name (string) catalog_table_name (string)
Name of the table to use as a catalog. Name of the table to use as a catalog.
auto_extend_select_list (boolean)
If True, select_list is automatically extended to have columns
used in group_by_list and order_by_list. It is useful when use
select_expression in inner query and use group_by_expression or
order_by_expression in outer query.
Deprecated parameters. Deprecated parameters.
extra_column_list (list of string) extra_column_list (list of string)
......
...@@ -64,7 +64,8 @@ class ISearchKeyCatalog(Interface): ...@@ -64,7 +64,8 @@ class ISearchKeyCatalog(Interface):
""" """
def buildEntireQuery(kw, query_table='catalog', ignore_empty_string=1, def buildEntireQuery(kw, query_table='catalog', ignore_empty_string=1,
limit=None, extra_column_list=None): limit=None, auto_extend_select_list=False,
extra_column_list=None):
""" """
Construct and return an instance of EntireQuery class from given Construct and return an instance of EntireQuery class from given
parameters by calling buildQuery. parameters by calling buildQuery.
...@@ -95,6 +96,11 @@ class ISearchKeyCatalog(Interface): ...@@ -95,6 +96,11 @@ class ISearchKeyCatalog(Interface):
- type cast (see SQL documentation of 'CAST') - type cast (see SQL documentation of 'CAST')
Sort will happen on given parameter name (its column if it's a column Sort will happen on given parameter name (its column if it's a column
name, corresponding virtual column otherwise - as for related keys). name, corresponding virtual column otherwise - as for related keys).
auto_extend_select_list (boolean)
If True, select_list is automatically extended to have columns
used in group_by_list and order_by_list. It is useful when use
select_expression in inner query and use group_by_expression or
order_by_expression in outer query.
Extra parameters are passed through to buildQuery. Extra parameters are passed through to buildQuery.
Backward compatibility parameters: Backward compatibility parameters:
...@@ -140,7 +146,8 @@ class ISearchKeyCatalog(Interface): ...@@ -140,7 +146,8 @@ class ISearchKeyCatalog(Interface):
def buildSQLQuery(query_table='catalog', REQUEST=None, def buildSQLQuery(query_table='catalog', REQUEST=None,
ignore_empty_string=1, only_group_columns=False, ignore_empty_string=1, only_group_columns=False,
limit=None, extra_column_list=None, limit=None, auto_extend_select_list=False,
extra_column_list=(),
**kw): **kw):
""" """
Return an SQLExpression-generated dictionary (see Return an SQLExpression-generated dictionary (see
......
...@@ -61,6 +61,7 @@ class ISQLExpression(Interface): ...@@ -61,6 +61,7 @@ class ISQLExpression(Interface):
where_expression_operator=None, where_expression_operator=None,
sql_expression_list=None, sql_expression_list=None,
select_dict=None, select_dict=None,
auto_extend_select_list=False,
limit=None, limit=None,
from_expression=None): from_expression=None):
""" """
...@@ -100,6 +101,11 @@ class ISQLExpression(Interface): ...@@ -100,6 +101,11 @@ class ISQLExpression(Interface):
Key is column alias. Key is column alias.
Value is column name, or Null. If it is Null, the alias will also be Value is column name, or Null. If it is Null, the alias will also be
used as column name. used as column name.
auto_extend_select_list (boolean)
If True, select_list is automatically extended to have columns
used in group_by_list and order_by_list. It is useful when use
select_expression in inner query and use group_by_expression or
order_by_expression in outer query.
limit (1-tuple, 2-tuple, other) limit (1-tuple, 2-tuple, other)
First item is the number of lines expected, second one if given is the First item is the number of lines expected, second one if given is the
offset of limited result list within the unlimited result list. offset of limited result list within the unlimited result list.
......
...@@ -728,27 +728,27 @@ class TestSQLCatalog(ERP5TypeTestCase): ...@@ -728,27 +728,27 @@ class TestSQLCatalog(ERP5TypeTestCase):
order_by_expression = sql_expression.getOrderByExpression() order_by_expression = sql_expression.getOrderByExpression()
self.assertNotEqual(order_by_expression, '') self.assertNotEqual(order_by_expression, '')
# ... and not sort by relevance # ... and not sort by relevance
self.assertFalse('MATCH' in order_by_expression, order_by_expression) self.assertEqual('`foo`.`fulltext`', order_by_expression)
# order_by_list on fulltext column + '__score__, resulting "ORDER BY" must be non-empty. # order_by_list on fulltext column + '__score__, resulting "ORDER BY" must be non-empty.
sql_expression = self.asSQLExpression({'fulltext': 'foo', sql_expression = self.asSQLExpression({'fulltext': 'foo',
'order_by_list': [('fulltext__score__', ), ]}) 'order_by_list': [('fulltext__score__', ), ]})
order_by_expression = sql_expression.getOrderByExpression() order_by_expression = sql_expression.getOrderByExpression()
self.assertNotEqual(order_by_expression, '') self.assertNotEqual(order_by_expression, '')
# ... and must sort by relevance # ... and must sort by relevance
self.assertTrue('MATCH' in order_by_expression, order_by_expression) self.assertEqual('foo_fulltext__score__', order_by_expression)
# ordering on fulltext column with sort order specified must preserve # ordering on fulltext column with sort order specified must preserve
# sorting by relevance. # sorting by relevance.
for direction in ('ASC', 'DESC'): for direction in ('ASC', 'DESC'):
sql_expression = self.asSQLExpression({'fulltext': 'foo', sql_expression = self.asSQLExpression({'fulltext': 'foo',
'order_by_list': [('fulltext__score__', direction), ]}) 'order_by_list': [('fulltext__score__', direction), ]})
order_by_expression = sql_expression.getOrderByExpression() order_by_expression = sql_expression.getOrderByExpression()
self.assertTrue('MATCH' in order_by_expression, (order_by_expression, direction)) self.assertEqual('foo_fulltext__score__ %s' % direction, order_by_expression)
# Providing a None cast should work too # Providing a None cast should work too
for direction in ('ASC', 'DESC'): for direction in ('ASC', 'DESC'):
sql_expression = self.asSQLExpression({'fulltext': 'foo', sql_expression = self.asSQLExpression({'fulltext': 'foo',
'order_by_list': [('fulltext__score__', direction, None), ]}) 'order_by_list': [('fulltext__score__', direction, None), ]})
order_by_expression = sql_expression.getOrderByExpression() order_by_expression = sql_expression.getOrderByExpression()
self.assertTrue('MATCH' in order_by_expression, (order_by_expression, direction)) self.assertEqual('foo_fulltext__score__ %s' % direction, order_by_expression)
def test_logicalOperators(self): def test_logicalOperators(self):
self.catalog(ReferenceQuery(ReferenceQuery(operator='=', default='AN ORB'), self.catalog(ReferenceQuery(ReferenceQuery(operator='=', default='AN ORB'),
...@@ -759,6 +759,42 @@ class TestSQLCatalog(ERP5TypeTestCase): ...@@ -759,6 +759,42 @@ class TestSQLCatalog(ERP5TypeTestCase):
operator='and'), operator='and'),
{'default': 'AN OR ORB'}) {'default': 'AN OR ORB'})
def test_auto_extend_select_list(self):
# by default select_list is not automatically extended by
# order_by_list or group_by_list.
sql_expression = self.asSQLExpression({
'order_by_list': [('default',),]})
select_dict = sql_expression.getSelectDict()
self.assertEqual({}, select_dict)
sql_expression = self.asSQLExpression({
'group_by_list': ['default',]})
select_dict = sql_expression.getSelectDict()
self.assertEqual({}, select_dict)
# select_list is extended if auto_extend_select_list is enabled.
sql_expression = self.asSQLExpression({
'order_by_list': [('default',),]},
auto_extend_select_list=True)
select_dict = sql_expression.getSelectDict()
self.assertEqual({'foo_default__ext__': '`foo`.`default`'}, select_dict)
sql_expression = self.asSQLExpression({
'group_by_list': ['default',]},
auto_extend_select_list=True)
select_dict = sql_expression.getSelectDict()
self.assertEqual({'foo_default__ext__': '`foo`.`default`'}, select_dict)
# fulltext score is automatically added in select_dict even if
# auto_extend_select_list is not enabled.
sql_expression = self.asSQLExpression({
'fulltext': 'foo',
'order_by_list': [('fulltext__score__',),]})
select_dict = sql_expression.getSelectDict()
self.assertEqual(['foo_fulltext__score__'], select_dict.keys())
sql_expression = self.asSQLExpression({
'fulltext': 'foo',
'order_by_list': [('fulltext__score__',),]},
auto_extend_select_list=True)
select_dict = sql_expression.getSelectDict()
self.assertEqual(['foo_fulltext__score__'], select_dict.keys())
def _searchTextInDictQuery(self, column): def _searchTextInDictQuery(self, column):
self.catalog(ReferenceQuery(ReferenceQuery( self.catalog(ReferenceQuery(ReferenceQuery(
ReferenceQuery(operator='>=', date=DateTime('2001/08/11')), ReferenceQuery(operator='>=', date=DateTime('2001/08/11')),
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment