Commit 33910d0a authored by R.W.Majeed's avatar R.W.Majeed

DataDialect: timezone for database timestamp with conversions

parent a9d96ac4
package de.sekmi.histream;
import java.io.IOException;
import java.time.Instant;
/**
......@@ -14,6 +15,7 @@ public interface ObservationExtractor {
/**
* Extract observations with a start time stamp between the specified limits.
* Only observations with the specified notations are extracted.
* TODO evaluate change from ObservationException to IOException
*
* @param start_min minimum time for observation start (inclusive)
* @param start_max maximum time for observation start (inclusive)
......@@ -21,5 +23,5 @@ public interface ObservationExtractor {
* @return supplier for the extracted observations. Must be closed after use.
* @throws ObservationException error (e.g. database failure)
*/
ObservationSupplier extract(Instant start_min, Instant start_max, Iterable<String> notations) throws ObservationException;
ObservationSupplier extract(Instant start_min, Instant start_max, Iterable<String> notations) throws IOException;
}
package de.sekmi.histream.i2b2;
import java.sql.Timestamp;
import java.time.Instant;
import java.time.ZoneId;
import java.time.ZoneOffset;
import de.sekmi.histream.AbnormalFlag;
import de.sekmi.histream.DateTimeAccuracy;
import de.sekmi.histream.Value;
/**
* Configuration of exact meaning of values in
* {@code observation_fact} table. E.g. how to
* store/interpret null values.
* <p>
* The function calls beginning with {@code encode} produce
* values which are stored in the database. The {@code decode} functions
* are used to decode the database values and produce a usable value.
*
* @author R.W.Majeed
*
......@@ -18,6 +28,9 @@ public class DataDialect {
private String nullModifierCd;
private String nullValueFlagCd;
private String nullValueTypeCd;
/** Timezone for timestamp / date time columns */
private ZoneId zoneId;
// TODO nullSexCd, nullInOutCd
public DataDialect(){
this.nullUnitCd = "@"; // technically, null is allowed, but the demodata uses both '@' and ''
......@@ -28,10 +41,14 @@ public class DataDialect {
this.nullValueTypeCd = "@"; // TODO check database
// null not allowed, use default
this.nullProviderId = "@";
this.zoneId = ZoneId.systemDefault();
}
void setDefaultProviderId(String providerId){
this.nullProviderId = providerId;
}
public void setTimeZone(ZoneId zone){
this.zoneId = zone;
}
public String getDefaultProviderId(){
return nullProviderId;
}
......@@ -43,7 +60,9 @@ public class DataDialect {
public String getNullLocationCd(){
return nullLocationCd;
}
public ZoneId getTimeZone(){
return zoneId;
}
public String getNullModifierCd(){
return nullModifierCd;
}
......@@ -53,6 +72,34 @@ public class DataDialect {
public String getNullValueTypeCd(){
return nullValueTypeCd;
}
public Timestamp encodeInstant(Instant instant){
if( instant == null ){
return null;
}else{
return Timestamp.from(instant.atZone(zoneId).toLocalDateTime().atOffset(ZoneOffset.UTC).toInstant());
}
}
public Timestamp encodeInstantPartial(DateTimeAccuracy instant){
if( instant == null ){
return null;
}else{
return encodeInstant(instant.toInstantMin());
}
}
public Instant decodeInstant(Timestamp timestamp){
if( timestamp == null ){
return null;
}else{
return timestamp.toInstant().atOffset(ZoneOffset.UTC).toLocalDateTime().atZone(zoneId).toInstant();
}
}
public DateTimeAccuracy decodeInstantPartial(Timestamp timestamp){
if( timestamp == null ){
return null;
}else{
return new DateTimeAccuracy(decodeInstant(timestamp));
}
}
private boolean isNullComparison(String value, String nullValue){
if( value == null ){
return true;
......
package de.sekmi.histream.i2b2;
import java.io.IOException;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
......@@ -14,10 +15,8 @@ import java.util.logging.Logger;
import javax.sql.DataSource;
import de.sekmi.histream.ObservationException;
import de.sekmi.histream.ObservationExtractor;
import de.sekmi.histream.ObservationFactory;
import de.sekmi.histream.ObservationSupplier;
import de.sekmi.histream.ext.Patient;
import de.sekmi.histream.ext.Visit;
......@@ -64,6 +63,7 @@ public class I2b2ExtractorFactory implements AutoCloseable, ObservationExtractor
ds = crc_ds;
dialect = new DataDialect();
}
public ObservationFactory getObservationFactory(){
return observationFactory;
}
......@@ -140,7 +140,7 @@ public class I2b2ExtractorFactory implements AutoCloseable, ObservationExtractor
* @throws SQLException error
*/
//@SuppressWarnings("resource")
public I2b2Extractor extract(Timestamp start_min, Timestamp start_max, Iterable<String> notations) throws SQLException{
I2b2Extractor extract(Timestamp start_min, Timestamp start_max, Iterable<String> notations) throws SQLException{
// TODO move connection and prepared statement to I2b2Extractor
PreparedStatement ps = null;
ResultSet rs = null;
......@@ -204,11 +204,11 @@ public class I2b2ExtractorFactory implements AutoCloseable, ObservationExtractor
}
@Override
public ObservationSupplier extract(Instant start_min, Instant start_max, Iterable<String> notations) throws ObservationException{
public I2b2Extractor extract(Instant start_min, Instant start_max, Iterable<String> notations) throws IOException{
try {
return extract(Timestamp.from(start_min), Timestamp.from(start_max), notations);
return extract(dialect.encodeInstant(start_min),dialect.encodeInstant(start_max), notations);
} catch (SQLException e) {
throw new ObservationException(e);
throw new IOException(e);
}
}
}
......@@ -26,7 +26,6 @@ import java.io.IOException;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.Timestamp;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Objects;
......@@ -55,6 +54,10 @@ import de.sekmi.histream.impl.AbstractObservationHandler;
* valtype_cd: N numeric, B stored in observation_blob, T text, '@' no value, 'NLP' NLP result xml objects.
* Undocumented but used in demodata: D: datetime "YYYY-MM-DD HH:mm" stored in tval_char, "YYYYMMDD.HHmm0" stored in nval_num.
* <p>
* Timestamp and datetime values are stored without timezone information. The timezone
* which should be used when reading/writing to database can be specified via
* the {@link DataDialect} param in {@link #open(Connection, DataDialect)}.
* <p>
* The most difficult part is handling the instance_num field.
* By default, i2b2 uses a four byte signed integer for instance_num. Incrementing
* instance_num for every record would lead eventually to a number overflow.
......@@ -64,7 +67,7 @@ import de.sekmi.histream.impl.AbstractObservationHandler;
* in any order. Therefore, we keep track of the maximum instance_num per encounter
* in the visit store (which caches visits anyways) and increase the instance_num only
* for observations with modifiers.
*
*
* @author R.W.Majeed
*
*/
......@@ -88,9 +91,7 @@ public class I2b2Inserter extends AbstractObservationHandler implements Observat
// initialize(config);
// }
public I2b2Inserter(){
}
private interface Preprocessor{
void preprocess(Observation fact)throws SQLException;
}
......@@ -273,7 +274,7 @@ public class I2b2Inserter extends AbstractObservationHandler implements Observat
insertFact.setString(4, dialect.encodeProviderId(o.getProviderId()));
// start_date
Objects.requireNonNull(o.getStartTime());
insertFact.setTimestamp(5, Timestamp.from(o.getStartTime().toInstantMin()));
insertFact.setTimestamp(5, dialect.encodeInstant(o.getStartTime().toInstantMin()));
insertFact.setString(6, (m==null)?dialect.getNullModifierCd():m.getConceptId());
insertFact.setInt(7, instanceNum);
......@@ -324,12 +325,12 @@ public class I2b2Inserter extends AbstractObservationHandler implements Observat
if( o.getEndTime() == null ){
insertFact.setTimestamp(13, null);
}else{
insertFact.setTimestamp(13, Timestamp.from(o.getEndTime().toInstantMin()));
insertFact.setTimestamp(13, dialect.encodeInstant(o.getEndTime().toInstantMin()));
}
// location_cd
insertFact.setString(14, dialect.encodeLocationCd(o.getLocationId()));
// download_date
insertFact.setTimestamp(15, Timestamp.from(o.getSource().getSourceTimestamp()));
insertFact.setTimestamp(15, dialect.encodeInstant(o.getSource().getSourceTimestamp()));
insertFact.setString(16, o.getSource().getSourceId());
insertFact.executeUpdate();
......
......@@ -22,9 +22,6 @@ package de.sekmi.histream.i2b2;
import java.sql.SQLException;
import java.sql.Timestamp;
import de.sekmi.histream.DateTimeAccuracy;
import de.sekmi.histream.Extension;
/**
* Extension with database connectivity.
......@@ -90,13 +87,7 @@ public abstract class PostgresExtension<T> implements Extension<T> {
// }else{
// return defaultFetchSize;
// }
// }
public static Timestamp inaccurateSqlTimestamp(DateTimeAccuracy dateTime){
if( dateTime == null )return null;
else return Timestamp.from(dateTime.toInstantMin());
}
// }
/**
* Write updates to disk.
......
......@@ -29,7 +29,6 @@ import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Enumeration;
......@@ -104,6 +103,8 @@ public class PostgresPatientStore extends PostgresExtension<I2b2Patient> impleme
private PreparedStatement selectAllIde;
private PreparedStatement deletePatientSource;
private PreparedStatement deleteMapSource;
private DataDialect dialect;
// /**
// * Construct new postgres patient store. In addition to properties
......@@ -149,12 +150,12 @@ public class PostgresPatientStore extends PostgresExtension<I2b2Patient> impleme
this.idSourceDefault = "HIVE";
this.idSourceSeparator = ':';
this.fetchSize = 1000;
// TODO add methods to change the configuration
}
public void open(Connection connection, String projectId) throws SQLException{
public void open(Connection connection, String projectId, DataDialect dialect) throws SQLException{
this.db = connection;
this.projectId = projectId;
this.dialect = dialect;
// require project id
Objects.requireNonNull(this.projectId, "non-null projectId required");
// this.autoInsertSourceId = "HS.auto";
......@@ -364,11 +365,11 @@ public class PostgresPatientStore extends PostgresExtension<I2b2Patient> impleme
private void updateStorage(I2b2Patient patient) throws SQLException {
synchronized( update ){
update.setString(1, patient.getVitalStatusCd());
update.setTimestamp(2, inaccurateSqlTimestamp(patient.getBirthDate()));
update.setTimestamp(3, inaccurateSqlTimestamp(patient.getDeathDate()));
update.setTimestamp(2, dialect.encodeInstantPartial(patient.getBirthDate()));
update.setTimestamp(3, dialect.encodeInstantPartial(patient.getDeathDate()));
update.setString(4, getSexCd(patient));
if( patient.getSourceTimestamp() != null ){
update.setTimestamp(5, Timestamp.from(patient.getSourceTimestamp()));
update.setTimestamp(5, dialect.encodeInstant(patient.getSourceTimestamp()));
}else{
update.setTimestamp(5, null);
}
......@@ -423,19 +424,10 @@ public class PostgresPatientStore extends PostgresExtension<I2b2Patient> impleme
// make sure that non-null vital code contains at least one character
if( vital_cd == null || vital_cd.length() == 0 )vital_cd = null;
DateTimeAccuracy birthDate = null;
DateTimeAccuracy deathDate = null;
// birth date
Timestamp ts = rs.getTimestamp(3);
if( ts != null ){
birthDate = new DateTimeAccuracy(ts.toInstant());
}
// death date
ts = rs.getTimestamp(4);
if( ts != null ){
deathDate = new DateTimeAccuracy(ts.toInstant());
}
DateTimeAccuracy birthDate = dialect.decodeInstantPartial(rs.getTimestamp(3));
DateTimeAccuracy deathDate = dialect.decodeInstantPartial(rs.getTimestamp(4));
;
// load sex
String sex_cd = rs.getString(5);
Sex sex = null;
......@@ -456,9 +448,9 @@ public class PostgresPatientStore extends PostgresExtension<I2b2Patient> impleme
}
I2b2Patient patient = new I2b2Patient(id, sex, birthDate, deathDate);
if( rs.getTimestamp(6) != null )
patient.setSourceTimestamp(rs.getTimestamp(6).toInstant());
if( rs.getTimestamp(6) != null ){
patient.setSourceTimestamp(dialect.decodeInstant(rs.getTimestamp(6)));
}
patient.setSourceId(rs.getString(7));
patient.setVitalStatusCd(vital_cd);
......@@ -504,7 +496,7 @@ public class PostgresPatientStore extends PostgresExtension<I2b2Patient> impleme
insertIde.setString(2, ids[0]);
insertIde.setInt(3, patient_num);
insertIde.setString(4, status);
insertIde.setTimestamp(5, Timestamp.from(source.getSourceTimestamp()));
insertIde.setTimestamp(5, dialect.encodeInstant(source.getSourceTimestamp()));
insertIde.setString(6, source.getSourceId());
insertIde.executeUpdate();
}
......
......@@ -29,7 +29,6 @@ import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Enumeration;
......@@ -88,7 +87,9 @@ public class PostgresVisitStore extends PostgresExtension<I2b2Visit> implements
private PreparedStatement selectMappingsAll;
private PreparedStatement deleteSource;
private PreparedStatement deleteMapSource;
private DataDialect dialect;
// /**
// * Create a visit store using configuration settings.
// * The project id must be specified with the key {@code project}.
......@@ -125,11 +126,12 @@ public class PostgresVisitStore extends PostgresExtension<I2b2Visit> implements
this.fetchSize = 1000;
this.rejectPatientChange = false;
}
public void open(Connection connection, String projectId) throws SQLException{
public void open(Connection connection, String projectId, DataDialect dialect) throws SQLException{
visitCache = new Hashtable<>();
idCache = new Hashtable<>();
this.projectId = projectId;
this.db = connection;
this.dialect = dialect;
// require project id
Objects.requireNonNull(this.projectId, "non-null projectId required");
db.setAutoCommit(true);
......@@ -309,11 +311,11 @@ public class PostgresVisitStore extends PostgresExtension<I2b2Visit> implements
private void updateStorage(I2b2Visit visit) throws SQLException {
synchronized( update ){
update.setString(1, visit.getActiveStatusCd());
update.setTimestamp(2, inaccurateSqlTimestamp(visit.getStartTime()));
update.setTimestamp(3, inaccurateSqlTimestamp(visit.getEndTime()));
update.setTimestamp(2, dialect.encodeInstantPartial(visit.getStartTime()));
update.setTimestamp(3, dialect.encodeInstantPartial(visit.getEndTime()));
update.setString(4, visit.getInOutCd());
update.setString(5, visit.getLocationId());
update.setTimestamp(6, Timestamp.from(visit.getSourceTimestamp()));
update.setString(5, dialect.encodeLocationCd(visit.getLocationId()));
update.setTimestamp(6, dialect.encodeInstant(visit.getSourceTimestamp()));
update.setString(7, visit.getSourceId());
// where encounter_num=visit.getNum()
......@@ -338,7 +340,7 @@ public class PostgresVisitStore extends PostgresExtension<I2b2Visit> implements
synchronized( insert ){
insert.setInt(1, visit.getNum() );
insert.setInt(2, visit.getPatientNum());
insert.setTimestamp(3, Timestamp.from(visit.getSourceTimestamp()));
insert.setTimestamp(3, dialect.encodeInstant(visit.getSourceTimestamp()));
insert.setString(4, visit.getSourceId());
insert.executeUpdate();
// other fields are not written, don't clear the dirty flag
......@@ -355,7 +357,7 @@ public class PostgresVisitStore extends PostgresExtension<I2b2Visit> implements
insertMapping.setString(5, ids[0]); // patient_ide_source
insertMapping.setTimestamp(6, Timestamp.from(visit.getSourceTimestamp()));
insertMapping.setTimestamp(6, dialect.encodeInstant(visit.getSourceTimestamp()));
insertMapping.setString(7, visit.getSourceId());
insertMapping.executeUpdate();
}
......@@ -373,18 +375,8 @@ public class PostgresVisitStore extends PostgresExtension<I2b2Visit> implements
// make sure that non-null vital code contains at least one character
if( active_status_cd != null && active_status_cd.length() == 0 )active_status_cd = null;
DateTimeAccuracy startDate = null;
DateTimeAccuracy endDate = null;
// birth date
Timestamp ts = rs.getTimestamp(4);
if( ts != null ){
startDate = new DateTimeAccuracy(ts.toInstant());
}
// death date
ts = rs.getTimestamp(5);
if( ts != null ){
endDate = new DateTimeAccuracy(ts.toInstant());
}
DateTimeAccuracy startDate = dialect.decodeInstantPartial(rs.getTimestamp(4));
DateTimeAccuracy endDate = dialect.decodeInstantPartial(rs.getTimestamp(5));
// load sex
String inout_cd = rs.getString(6);
......@@ -407,8 +399,8 @@ public class PostgresVisitStore extends PostgresExtension<I2b2Visit> implements
visit.setStatus(status);
visit.setActiveStatusCd(active_status_cd);
visit.setLocationId(rs.getString(7));
visit.setSourceTimestamp(rs.getTimestamp(8).toInstant());
visit.setLocationId(dialect.decodeLocationCd(rs.getString(7)));
visit.setSourceTimestamp(dialect.decodeInstant(rs.getTimestamp(8)));
visit.setSourceId(rs.getString(9));
// additional fields go here
......
package de.sekmi.histream.i2b2;
import java.sql.Timestamp;
import java.time.Instant;
import java.time.LocalDateTime;
import java.time.ZoneId;
import static org.junit.Assert.*;
import org.junit.Test;
import de.sekmi.histream.DateTimeAccuracy;
public class TestDataDialect {
@Test
public void verifySqlTimestampConversions(){
DataDialect dialect = new DataDialect();
dialect.setTimeZone(ZoneId.of("Asia/Shanghai"));
LocalDateTime local = LocalDateTime.of(2001,2,3,4,5);
System.out.println(local.toString());
Timestamp ts = Timestamp.valueOf(local);
System.out.println(ts.toInstant());
Instant inst = Instant.parse("2001-02-03T04:05:06Z");
DateTimeAccuracy da = new DateTimeAccuracy(inst);
System.out.println(inst);
ts = dialect.encodeInstant(inst);
assertEquals(ts, dialect.encodeInstantPartial(da));
System.out.println(ts.toInstant());
Instant b = dialect.decodeInstant(ts);
System.out.println(b);
assertEquals(inst, b);
}
}
package de.sekmi.histream.i2b2;
import java.text.ParseException;
import java.time.ZoneOffset;
import java.time.temporal.ChronoField;
import java.time.temporal.ChronoUnit;
......@@ -66,14 +67,14 @@ public class TestI2b2Patient {
I2b2Patient v = createPatientWithTimestamps();
v.setVitalStatusCd("UH");
assertEquals(ChronoUnit.HOURS, v.getBirthDate().getAccuracy());
assertEquals(4, v.getBirthDate().get(ChronoField.HOUR_OF_DAY));
assertEquals(4, v.getBirthDate().toInstantMin().atOffset(ZoneOffset.UTC).toLocalDateTime().get(ChronoField.HOUR_OF_DAY));
assertNull(v.getDeathDate());
v = createPatientWithTimestamps();
v.setVitalStatusCd("RL");
assertNull(v.getBirthDate());
assertEquals(ChronoUnit.HOURS, v.getDeathDate().getAccuracy());
assertEquals(4, v.getDeathDate().get(ChronoField.HOUR_OF_DAY));
assertEquals(4, v.getDeathDate().toInstantMin().atOffset(ZoneOffset.UTC).toLocalDateTime().get(ChronoField.HOUR_OF_DAY));
}
@Test
public void verifyMonthAndYearAccuracy(){
......
package de.sekmi.histream.i2b2;
import java.text.ParseException;
import java.time.ZoneOffset;
import java.time.temporal.ChronoField;
import java.time.temporal.ChronoUnit;
......@@ -13,7 +14,7 @@ public class TestI2b2Visit {
private DateTimeAccuracy createAccurateTimestamp(){
try {
return DateTimeAccuracy.parsePartialIso8601("2001-02-03T04:05:06");
return DateTimeAccuracy.parsePartialIso8601("2001-02-03T04:05:06Z");
} catch (ParseException e) {
throw new AssertionError();
}
......@@ -70,14 +71,14 @@ public class TestI2b2Visit {
I2b2Visit v = createVisitWithTimestamps();
v.setActiveStatusCd("UH");
assertEquals(ChronoUnit.HOURS, v.getStartTime().getAccuracy());
assertEquals(4, v.getStartTime().get(ChronoField.HOUR_OF_DAY));
assertEquals(4, v.getStartTime().toInstantMin().atOffset(ZoneOffset.UTC).get(ChronoField.HOUR_OF_DAY));
assertNull(v.getEndTime());
v = createVisitWithTimestamps();
v.setActiveStatusCd("RL");
assertNull(v.getStartTime());
assertEquals(ChronoUnit.HOURS, v.getEndTime().getAccuracy());
assertEquals(4, v.getEndTime().get(ChronoField.HOUR_OF_DAY));
assertEquals(4, v.getEndTime().toInstantMin().atOffset(ZoneOffset.UTC).get(ChronoField.HOUR_OF_DAY));
}
@Test
public void verifyMonthAndYearAccuracy(){
......
......@@ -32,7 +32,7 @@ public class TestPostgresPatientStore implements Closeable {
public void open(String host, int port, String user, String password, String projectId) throws ClassNotFoundException, SQLException{
store = new PostgresPatientStore();
store.open(DriverManager.getConnection("jdbc:postgresql://"+host+":"+port+"/i2b2", user, password),projectId);
store.open(DriverManager.getConnection("jdbc:postgresql://"+host+":"+port+"/i2b2", user, password),projectId, new DataDialect());
}
public PostgresPatientStore getStore(){return store;}
......@@ -42,9 +42,9 @@ public class TestPostgresPatientStore implements Closeable {
store.close();
}
private void open()throws Exception{
open("localhost",15432,"i2b2demodata", "demodata","demo");
}
// private void open()throws Exception{
// open("localhost",15432,"i2b2demodata", "demodata","demo");
// }
public static void main(String args[]) throws Exception{
TestPostgresPatientStore test = new TestPostgresPatientStore();
// test.open();
......
......@@ -33,7 +33,7 @@ public class TestPostgresVisitStore implements Closeable {
public void open(String host, int port) throws ClassNotFoundException, SQLException{
store = new PostgresVisitStore();
store.open(DriverManager.getConnection("jdbc:postgresql://"+host+":"+port+"/i2b2", "i2b2demodata", "demodata"), "demo");
store.open(DriverManager.getConnection("jdbc:postgresql://"+host+":"+port+"/i2b2", "i2b2demodata", "demodata"), "demo", new DataDialect());
}
private void open()throws Exception{
......
......@@ -3,6 +3,7 @@ package de.sekmi.histream.etl.config;
import java.io.IOException;
import java.math.BigDecimal;
import java.net.URL;
import java.time.ZoneOffset;
import java.time.temporal.ChronoField;
import javax.xml.bind.JAXB;
......@@ -41,7 +42,7 @@ public class TestReadTables {
try( RecordSupplier<PatientRow> s = ds.patientTable.open(of,ds.getMeta()) ){
PatientRow r = s.get();
Assert.assertEquals("p1", r.getId());
Assert.assertEquals(2003, r.getBirthDate().get(ChronoField.YEAR));
Assert.assertEquals(2003, r.getBirthDate().toInstantMin().atOffset(ZoneOffset.UTC).get(ChronoField.YEAR));
}
}
......@@ -50,7 +51,7 @@ public class TestReadTables {
try( RecordSupplier<VisitRow> s = ds.visitTable.open(of,ds.getMeta()) ){
VisitRow r = s.get();
Assert.assertEquals("v1", r.getId());
Assert.assertEquals(2013, r.getStartTime().get(ChronoField.YEAR));
Assert.assertEquals(2013, r.getStartTime().toInstantMin().atOffset(ZoneOffset.UTC).get(ChronoField.YEAR));
}
}
......
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